1280927 - [wasm] Hit MOZ_CRASH(Compiler bug: expected float on stack) at js/src/asmjs/WasmBaselineCompile.cpp:1393

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision e9723c6c6136+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug). To reproduce, you can run the following code in the JS shell (running with --wasm-always-baseline might be necessary):

var data = os.file.readFile(file, 'binary');
Wasm.instantiateModule(new Uint8Array(data.buffer));



Backtrace:

==12167==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002a131b4 bp 0x7ffe1c254ff0 sp 0x7ffe1c254f40 T0)
    #0 0x2a131b3 in js::wasm::BaseCompiler::popF32(js::wasm::BaseCompiler::Stk&, js::wasm::BaseCompiler::RegF32) js/src/asmjs/WasmBaselineCompile.cpp:1380:13
    #1 0x29ccc51 in js::wasm::BaseCompiler::popF32() js/src/asmjs/WasmBaselineCompile.cpp:1404:13
    #2 0x29ccc51 in js::wasm::BaseCompiler::emitSqrtF32() js/src/asmjs/WasmBaselineCompile.cpp:4091
    #3 0x29e904c in js::wasm::BaseCompiler::emitBody() js/src/asmjs/WasmBaselineCompile.cpp:5921:13
    #4 0x29f0745 in js::wasm::BaseCompiler::emitFunction() js/src/asmjs/WasmBaselineCompile.cpp:6215:10
    #5 0x29f47a2 in js::wasm::BaselineCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmBaselineCompile.cpp:6482:10
    #6 0x72c845 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3477:16
    #7 0x6afca0 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:824:14
    #8 0x645e3d in DecodeFunctionBody(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/Wasm.cpp:940:12
    #9 0x645e3d in DecodeCodeSection(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/Wasm.cpp:968
    #10 0x645e3d in DecodeModule(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::wasm::ShareableBytes const&, JS::MutableHandle<js::ArrayBufferObject*>) js/src/asmjs/Wasm.cpp:1141
    #11 0x63bc72 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/Wasm.cpp:1249:27
    #12 0x59a42e in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5226:14
    #13 0x1ea21d1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235:15
 [...]
    #26 0x461088 in _start (/home/ubuntu/build/build/js+0x461088)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/asmjs/WasmBaselineCompile.cpp:1380:13 in js::wasm::BaseCompiler::popF32(js::wasm::BaseCompiler::Stk&, js::wasm::BaseCompiler::RegF32)
==12167==ABORTING

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

This looks like a failure to handle a "value" that flows out of a computation that never results in anything because it branches out past the value consumer.  The code looks like this:

1	(loop $label0 $label1
2	      (br_if $label0 (i32.gt_u (get_local $var0) (i32.const 19)))
3	      (set_local $var0 (i32.add (get_local $var0) (i32.const 1)))
4	      (f32.trunc (f32.trunc (f32.sqrt (loop (br $label1)))))
5	      (nop))

It is the branch from within the loop on line 4 out past the loop that starts on line 1 that renders the code outside the loop on line 4 moot.

When the Wasm-Bbaseline compiler was written I did not duplicate the "if (deadCode()) ..." conditions in the Wasm-Ion compiler, interpreting them as an optimization that I did not need, but I now see they are probably meant to handle unreachable-value situations.  This is probably a somewhat deep bug.  It is likely that several of the other bugs that were filed along with this one are for the same problem.

Comment 6

[Lars T Hansen [:lth]]

I don't have a test case here.  After other, simple fixes (1280933, 1280934, 1280926, 1280930, 1280921, 1281131) the test case for this bug already passes.  The test case for bug 1280929 passes only with the patch for this bug, but that test case is basically gibberish bytecode following one UNREACHABLE and I don't particularly want to include it here.

With the patch for this bug, all test cases pass and AngryBots still runs, which hopefully counts for something.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1280927-skip-unreachable.patch

Skip dead code during compilation.

The logic is mostly straightforward, with a deadCode_ flag in BaseCompiler that is set at RETURN, BR, BR_TABLE, and UNREACHABLE, and is checked by all emitters.  The hard part is computing the value of that flag at control point joins: we're in dead code after the join if no live paths flow to the join.

(After this patch we can probably clean up the cases in popI32() et al which will allow the presence of a NONE value on the stack, since those cases were there to handle unreachable code, but I did not want to clutter this patch with that cleanup.)

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8764246 [details] [diff] [review]
bug1280927-skip-unreachable.patch

Redirecting review to offload Benjamin.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8764246 [details] [diff] [review]
bug1280927-skip-unreachable.patch

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

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +310,5 @@
>          PooledLabel* otherLabel;        // Used for the "else" branch of if-then-else
>          uint32_t framePushed;           // From masm
>          uint32_t stackSize;             // Value stack height
> +        bool deadOnArrival;             // deadCode_ was set on entry to the region
> +        bool deadThenBranch;            // deadCode_ was set on exit from "then"

Since these fields aren't present in WasmIonCompile.cpp, I was trying to recall how Ion addressed this and I remembered that the nullness of the MBasicBlock*'s that get passed around end up serving dual-duty as the "in dead code" bool.  That gave me an idea which might make this deadness-tracking a bit more explicit and perhaps avoid bugs in the future: what if instead of having a Nothing/unused ControlItem, you had an enum class DeadCode { True, False } that was the value and this was propagated everywhere.  I'm not positive this is a win, but maybe it's worth trying to see?

Comment 10

[Lars T Hansen [:lth]]

Luke, I don't think I understand that remark at all.

Comment 11

[Luke Wagner [:luke]]

In WasmIonCompile, a null MBasicBlock* indicates that path is dead and MBasicBlock* is the ControlItem (typedef inside IonCompilePolicy).  We don't have basic blocks in baseline, but could the baseline instead have an `enum class DeadCode { True, False }` and use that as ControlItem.  The advantage I was imagining was greater symmetry with WasmIonCompile such that everywhere WasmIonCompile would branch on a basic block being null, WasmBaselineCompile would branch on DeadCode::True.

Comment 12

[Luke Wagner [:luke]]

Actually, I remembered that there is already a future TODO to use ExprIter to maintain the Control stack instead of having BaseCompiler maintain its own side ctl_ stack, so probably what I'm asking is orthogonal to the fix in this patch.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8764246 [details] [diff] [review]
bug1280927-skip-unreachable.patch

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

In the future, it'd be cool if ExprIter could track inDeadCode() itself so that both IonCompile and BaselineCompile could use it in a symmetric manner.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #13)
> Comment on attachment 8764246 [details] [diff] [review]
> bug1280927-skip-unreachable.patch
> 
> Review of attachment 8764246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the future, it'd be cool if ExprIter could track inDeadCode() itself so
> that both IonCompile and BaselineCompile could use it in a symmetric manner.

Yes, I think that would be a winner.  As you've observed, there are some tricky bits in the BaseCompiler patch, and as I remember it the logic in the Ion compiler is far from obvious.  Given the current type system every Wasm compiler will need to deal with dead code, so centralizing the logic seems desirable.

Comment 15

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c56c98ab5f996638f6790d65063039f2c357ff2
Bug 1280927 - skip unreachable code. r=luke

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9c56c98ab5f9