1743715 - MBitNot::foldsTo: don't assert when given an Int64 operand

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

For the last few days, on my local win10/x64 builds, when I capture a profile I get an assertion that crashes Firefox.

STR:

1. Build firefox locally from a recent central. My mozconfig contains ac_add_options --enable-debug, ac_add_options --enable-debug-symbols, ac_add_options --enable-optimize. (I don't know if Nightly shows the issue as well, you may want to try first.)
2. ./mach run
3. (If the profiled toolbar icon is not installed yet) Visit https://profiler.firefox.com , click "Enable Firefox Profiler Menu Button".
4. Start and stop the profiler (click on profiler button twice, or press ctrl+shift+1 then ctrl+shift+2).

A new tab should open, showing a short animation, then the profiler page saying "waiting for symbols [...]" at the top.
After a few seconds, the assertion appears in the terminal, then Firefox crashes.

Assertion failure: type() == MIRType::Int32, at js/src/jit/MIR.cpp:3159
#01: js::jit::MBitNot::foldsTo (js\src\jit\MIR.cpp:3159)
#02: js::jit::ValueNumberer::visitDefinition (js\src\jit\ValueNumbering.cpp:749)
#03: js::jit::ValueNumberer::visitBlock (js\src\jit\ValueNumbering.cpp:1010)
#04: js::jit::ValueNumberer::visitDominatorTree (js\src\jit\ValueNumbering.cpp:1064)
#05: js::jit::ValueNumberer::visitGraph (js\src\jit\ValueNumbering.cpp:1103)
#06: js::jit::ValueNumberer::run (js\src\jit\ValueNumbering.cpp:1271)
#07: js::jit::OptimizeMIR (js\src\jit\Ion.cpp:1183)
#08: js::wasm::IonCompileFunctions (js\src\wasm\WasmIonCompile.cpp:6036)
#09: ExecuteCompileTask (js\src\wasm\WasmGenerator.cpp:726)
#10: js::wasm::CompileTask::runHelperThreadTask (js\src\wasm\WasmGenerator.cpp:758)
[...]

Talking to @mstange, he recently worked on some wasm code that may be the one triggering this assertion: Bug 1742855, patch https://phabricator.services.mozilla.com/D132077

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Update:
I've reproduced it with a local build on Linux/x64 (in a VM).
But it's not happening in Official Nightly release there (it's a MOZ_ASSERT, so I guess it's just not tested; That's a bit scary if the rest of the code relies on that.)

And it's not happening in a local build on macos/M1.

Comment 2

[Lars T Hansen [:lth]]

Julian has been in MBitNot::foldsTo very recently, and may have broken something.

Comment 3

[Julian Seward [:jseward]]

Attachment: failure stack

This is fallout from bug 1741392. What happened is fairly clear: the patch there creates a MBitNot node of type Int64, which is at some point offered up to MBitNot::foldsTo to see if it can be folded into something simpler. But MBitNot::foldsTo is only expecting 32-bit-typed nodes, so asserts.

So far I can only get it to fail on Google Earth (stack attached). Looking at the stack, the failure happens during GVN. Attempts to create a small test case have so far failed. Given that it fails in GVN, one might hope it might be triggered by a test of the form

   (i64.mul
       (i64.xor (get_local $p1) (i64.const 0xFFFFFFFFFFFFFFFF))
       (i64.xor (get_local $p1) (i64.const 0xFFFFFFFFFFFFFFFF)))

where GVN would try to take out the duplicated term, but that is handled fine. Maybe it depends on the relative ordering of the xor->not transformation and GVN.

Comment 4

[Julian Seward [:jseward]]

The obvious fix is for MBitNot::foldsTo to return immediately if given an Int64-typed input. That at least de-crashes it, and with no loss of performance, since 64-bit MBitNot wasn't being folded anyway. The better fix is extend the folding rule to handle 64-bit cases. But I don't want to do that without a standalone test case.

Comment 5

[Julian Seward [:jseward]]

Attachment: Bug 1743715 - `MBitNot::foldsTo`: don't assert when given an Int64 operand. r=lth.

Bug 1741392 introduced a transformation which creates Int64-typed MBitNot
nodes. Unfortunately MBitNot::foldsTo asserts that its operand is Int32,
and the assertion was not updated. This fix returns from MBitNot::foldsTo
immediately in such a case. No folding happens, but it doesn't happen at the
moment anyway, so there's no perf loss. A longer term fix would be to extend
the routine to handle the Int64 case properly.

MBitNot::writeRecoverData also assumes -- although without an assertion --
that the operand has Int32 type. However, this is a JS-only path, so we never
expect to see Int64 here. An assertion has nevertheless been added for safety.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a942543866a
`MBitNot::foldsTo`: don't assert when given an Int64 operand.  r=lth.

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0a942543866a