1920594 - Broken frame pointer unwinding in wasm "builtin thunks" on aarch64 from tagging ExitFP

Status: NEW

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Markus Stange [:mstange]]

When profiling WebAssembly workloads in Spidermonkey with a native profiler such as samply, some stacks are broken.

For better stacks, the frame pointer register (x29 on aarch64) should always point to a valid frame record, i.e. a valid pair of "caller frame pointer" + "return address".

x29 is the frame pointer register, x30 is the return address register (lr).

Here's the generated code from a wasm builtin thunk on aarch64:

sub sp, sp, #0x10
str x30, [sp, #0x8]
str x29, [sp]
mov x29, sp
ldr x30, [x23, #0x20]
ldr x30, [x30, #0xf8]
mov w16, #0xa5
str w16, [x30, #0x78]
orr x29, x29, #0x1
str x29, [x30, #0x70]
and x29, x29, #0xfffffffffffffffe
mov x28, sp
mov x16, #0xf0b4
movk x16, #0x36b, lsl #16
movk x16, #0x1, lsl #32
blr x16
ldr x30, [x23, #0x20]
ldr x30, [x30, #0xf8]
mov x16, #0x0
str x16, [x30, #0x70]
mov w16, #0x0
str w16, [x30, #0x78]
ldr x30, [sp, #0x8]
ldr x29, [sp]
add sp, sp, #0x10
mov x28, sp
ret

The unwinding failures happen in the two instructions immediately following the orr x29, x29, #0x1 instruction:

str x29, [x30, #0x70]                ; 1602 broken samples out of ~200000
and x29, x29, #0xfffffffffffffffe    ; 62 broken samples out of ~200000

These instructions appear to be generated by wasm::SetExitFP.

Comment 1

[Ryan Hunt [:rhunt]]

We'd need another scratch register to be able to tag the ExitFP without clobbering the FramePointer as we're doing here. I think we might be able to get that. Otherwise I've always wondered why the JitActivation::exitFP_ needs to be tagged and we couldn't have two fields jsExitFP_ and wasmExitFP_. Jan do you know if there's a reason for this?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Ryan Hunt [:rhunt] from comment #1)

Otherwise I've always wondered why the JitActivation::exitFP_ needs to be tagged and we couldn't have two fields jsExitFP_ and wasmExitFP_. Jan do you know if there's a reason for this?

Wasm clears the exitFP by storing nullptr to it (in ClearExitFP). For JS we don't do this and we can leave a stale exit-FP stored in the activation after returning to JIT code.

I think your two fields idea would work as long as Wasm always clears its wasmExitFP_ after returning from C++. The invariant would be that if both jsExitFP_ and wasmExitFP_ are non-nullptr, we take the wasmExitFP_ because that one is guaranteed to be up-to-date.