Closed Bug 1308402 Opened 6 years ago Closed 6 years ago

Wasm baseline jit: crash on AngryBots

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

This regression was observed by Benjamin during the work week.

CallExport was implicated in the crash report but that's all the data we have so far.
Priority: -- → P3
This is a problem with the Tls register having a garbage value in the function prologue.
Priority: P3 → P1
This in turn appears to be caused by a function loading a value into the Tls register before a call from the wrong stack slot; it loads it from 0x28(rsp) but should almost certainly have loaded it from 0x30(rsp), where the Tls value was preserved upon entry.
This is a bug in the stack accounting in BaseCompiler::emitCallIndirect() that probably appeared as a consequence of handling both "old style" and "new style" calls.  The call to stk_.popCopy() only pops the compiler's value stack, not also the CPU stack; as a consequence, the accounting for stackUsed must always use numArgs + 1, and the stack usage must be computed before the popCopy().
Depends on: 1311433
Fairly straightforward local adjustment: just make sure to always pop the same amount from the CPU stack.

This sits on top of the current (non-reviewed) patch for bug 1311433 but it wouldn't be hard to land it independently if necessary.
Attachment #8803321 - Flags: review?(bbouvier)
Comment on attachment 8803321 [details] [diff] [review]
bug1308402-pop-it-all.patch

Review of attachment 8803321 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense; thanks!

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +5774,2 @@
>      uint32_t numArgs = sig.args().length();
> +    size_t stackSpace = stackConsumed(numArgs+1);

ubernit: can you add spaces between the + operands please?

@@ +5803,5 @@
>  
>      // TODO / OPTIMIZE: It would be better to merge this freeStack()
>      // into the one in endCall, if we can.
>  
> +    popValueStackBy(oldStyle ? numArgs+1 : numArgs);

Too bad we can't just popValueStackBy(numArgs + 1) all the time, by not calling popCopy() above but just a peek(1) (fwiw it seems this can't be done because emitCallArgs expect the args to be at the top of the stack).

Instead, could we maybe shorten the big new comment above, and here just say

// For new style calls, the callee has been already popped above.
Attachment #8803321 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2bb3582f357768a370686e26cca50698e225055
Bug 1308402 - pop the CPU stack properly after new-style call-indirect. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/b2bb3582f357
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.