1328127 - [wasm] Assertion failure: ins->isGoto(), at js/src/jit/IonAnalysis.cpp:767

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision df6ef96fc616+ (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');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==20336==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000193e3e3 bp 0x7ffdd20260b0 sp 0x7ffdd2026050 T0)
    #0 0x193e3e2 in js::jit::MTest* js::jit::MDefinition::to<js::jit::MTest>() js/src/jit/MIR.h:892:9
    #1 0x193e3e2 in js::jit::MDefinition::toTest() js/src/jit/MIR.h:909
    #2 0x193e3e2 in UpdateTestSuccessors(js::jit::TempAllocator&, js::jit::MBasicBlock*, js::jit::MDefinition*, js::jit::MBasicBlock*, js::jit::MBasicBlock*, js::jit::MBasicBlock*) js/src/jit/IonAnalysis.cpp:747
    #3 0x17c188d in MaybeFoldConditionBlock(js::jit::MIRGraph&, js::jit::MBasicBlock*) js/src/jit/IonAnalysis.cpp:882:9
    #4 0x17c188d in js::jit::FoldTests(js::jit::MIRGraph&) js/src/jit/IonAnalysis.cpp:920
    #5 0x17a1fbf in js::jit::OptimizeMIR(js::jit::MIRGenerator*) js/src/jit/Ion.cpp:1570:14
    #6 0xe4b2cb in js::wasm::IonCompileFunction(js::wasm::CompileTask*) js/src/wasm/WasmIonCompile.cpp:3766:14
    #7 0xd9dc7a in js::wasm::CompileFunction(js::wasm::CompileTask*) js/src/wasm/WasmGenerator.cpp:1151:16
    #8 0xd9d54b in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/wasm/WasmGenerator.cpp:931:14
    #9 0xd52d72 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/wasm/WasmCompile.cpp:60:12
    #10 0xd52d72 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:88
    #11 0xd52d72 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:124
    #12 0xf029ba in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:399:27
    #13 0x606340 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5659:14
    #14 0xab8836 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:239:15

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

This is a good find: MaybeFoldConditionBlock tries to fold its desired pattern assuming that all blocks having only one successor are terminated by a MGoto or MTest, not a MTableSwitch. Because of the manual jump threading and back-patching of edges in Baldr, we generate a MTableSwitch all the time and never get back to a MGoto if all the cases actually branch to the same block.

A few ways to solve this:
1. just add a check in MaybeFoldConditionBlock to *not* trigger when we have a table switch
2. implement what's needed in MaybeFoldConditionBlock
3. have Baldr transform switches with only one successor into a goto

Solution 1 would be good for an uplift and the least risky; solution 3 would be the best one in general but slightly more risky. I'll look into it though.

Textual test case (wasmBinaryToText Just Worked here \o/):

(module
  (func $f (param $p i32)
    block $out
      i32.const 0
      if
        i32.const 1
        tee_local $p
        br_table $out $out
      end
    end
    get_local $p
    br_if 0
  )
)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: brtable.patch

I was looking in the wrong direction for 3; it's actually a breeze.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8823736 [details] [diff] [review]
brtable.patch

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

Nice work!

::: js/src/wasm/WasmIonCompile.cpp
@@ +1844,5 @@
>  
>      MDefinition* maybeValue = IsVoid(type) ? nullptr : value;
>  
> +    // If all the targets are the same, or there are no targets, we can just
> +    // use a goto.

I'd add to this comment: "This is not just an optimization: MaybeFoldConditionBlock assumes that tables have more than one successor."

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe0b5c910f7
Fold br_table with all-same-successors into a goto in wasm; r=luke

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: brtable.patch

Approval Request Comment
[Feature/Bug causing the regression]: wasm initial release
[User impact if declined]: corruption of generated code, probably exploitable
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: introduces an optimization (=> increases risk) but also removes a wasm special-case in compilation
[String changes made/needed]: n/a

Carrying forward r+ with updated comment text.

Comment 7

[Julien Cristau [:jcristau]]

track this wasm issue for 52

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8824009 [details] [diff] [review]
brtable.patch

wasm codegen fix, aurora52+

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1225f99f2ba9