1305318 - Assertion failure: producer_ != nullptr, at js/src/jit/MIR.h:273

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision eb314c69ae72+ (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:

==25670==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000d63b61 bp 0x7ffeaf5cbb90 sp 0x7ffeaf5cbb80 T0)
    #0 0xd63b60 in js::jit::MDefinition::addUseUnchecked(js::jit::MUse*) js/src/jit/MIR.h:840:9
    #1 0x2be96bb in js::jit::MAryControlInstruction<2ul, 0ul>::initOperand(unsigned long, js::jit::MDefinition*) js/src/jit/MIR.h:2907:9
    #2 0x2be96bb in js::jit::MAsmJSReturn::MAsmJSReturn(js::jit::MDefinition*, js::jit::MDefinition*) js/src/jit/MIR.h:13622
    #3 0x2be96bb in js::jit::MAsmJSReturn* js::jit::MAsmJSReturn::New<js::jit::MDefinition*&, js::jit::MAsmJSParameter*&>(js::jit::TempAllocator&, js::jit::MDefinition*&, js::jit::MAsmJSParameter*&) js/src/jit/MIR.h:13628
    #4 0x2be96bb in (anonymous namespace)::FunctionCompiler::returnExpr(js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1118
    #5 0x2be96bb in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3760
    #6 0x2c25783 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3798:16
    #7 0x2b9f030 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #8 0x2b46a40 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:12
    #9 0x2b46a40 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1134
    #10 0x2b46a40 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1380
    #11 0x667119 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:361:27
    #12 0x5b57c8 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Christian, can you update your template: instead of

Wasm.instantiateModule(new Uint8Array(data.buffer));

it should be:

new WebAssembly.Instance(new WebAssembly.Module(data.buffer));


(I'm investigating the bug)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Dan, not too sure about the fix, but here's what happens:

- there's a (void) loop
- there's a brTable within this loop, which breaks out of this loop (to the function body, returning i32) and to this loop header (void).
- the special casing in the binary iterator's readBrTableEntry overrides the type/value of the br_table.

I'm not entirely sure where this is this special casing; it doesn't seem necessary, since when removing it all the tests pass (this one including -- by throwing an error that i32 != void) but I might be missing something.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(special casing *of Loop* [...] overrides type/value of the br_table to void/nullptr)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Also: this test makes wasmBinaryToText crash in a different way. After the main test case, there's rubbish unreachable code; the binary iterator knows not to push and pop in this case, but the binary-to-text transform does not.

Can we rely on WasmBinaryIterator::reachable_ and not push/pop in WasmBinaryToText, if it's false?

Comment 6

[Christian Holler (:decoder)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Christian, can you update your template: instead of
> 
> Wasm.instantiateModule(new Uint8Array(data.buffer));
> 
> it should be:
> 
> new WebAssembly.Instance(new WebAssembly.Module(data.buffer));
> 
> 
> (I'm investigating the bug)

Done :)

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8794903 [details] [diff] [review]
fix.patch

After IRC discussion, clearing review: the patch is incorrect and needs some rewriting.

Comment 8

[Dan Gohman [:sunfish]]

Here's a testcase which shows the difference:

wasmFailValidateText(`
(module
  (func (result i32)
    (loop i32
      (i32.const 0)
      (br_table 1 0 (i32.const 15))
    )
  )
)`, mismatchError("i32", "void"));

which differs from the testcase in the patch only by adding "i32" to the loop.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1305318.patch

Sorry for the slowness here, the fix now sounds pretty easy and obvious.

I've added three test cases and compared with ml-proto, which behaved the same as we do now (except for the second one which is a syntax error there, because ml-proto doesn't accept the syntax "loop i32").

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8796560 [details] [diff] [review]
1305318.patch

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

::: js/src/asmjs/WasmBinaryIterator.h
@@ +1136,5 @@
>          }
>      }
>  
> +    if (knownType != ExprType::Limit && knownType != ExprType::Void)
> +        return typeMismatch(knownType, ExprType::Void);

This should be inside the "if (MOZ_LIKELY(reachable_))" body so that we don't issue spurious type errors in unreachable code.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks! I was trying to make a test case failing to behave correctly (dead code), but I actually think this can't happen:

- either a br_table and all its entries are reachable, or none of them are.
- if they're not reachable, knownType == ExprType::Limit in each call to readBrTableEntry.

However this is not trivial to infer from context, so I'll just move the check within the `if (reachable_)` part as you suggested.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f95c86c04c
Reject a typed jump to the top of a loop in a br_table; r=sunfish

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56f95c86c04c