Closed Bug 1328127 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files, 1 obsolete file)

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
Attached file Testcase
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
  )
)
Attached patch brtable.patch (obsolete) — Splinter Review
I was looking in the wrong direction for 3; it's actually a breeze.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8823736 - Flags: review?(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."
Attachment #8823736 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe0b5c910f7
Fold br_table with all-same-successors into a goto in wasm; r=luke
Attached patch brtable.patchSplinter Review
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.
Attachment #8823736 - Attachment is obsolete: true
Attachment #8824009 - Flags: review+
Attachment #8824009 - Flags: approval-mozilla-aurora?
track this wasm issue for 52
https://hg.mozilla.org/mozilla-central/rev/0fe0b5c910f7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(bbouvier)
Comment on attachment 8824009 [details] [diff] [review]
brtable.patch

wasm codegen fix, aurora52+
Flags: needinfo?(bbouvier)
Attachment #8824009 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.