1280933 - [wasm] Assertion failure: isAvailable(r), at js/src/asmjs/WasmBaselineCompile.cpp:537

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:

==12391==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002a0fe17 bp 0x7ffe13f75740 sp 0x7ffe13f75730 T0)
    #0 0x2a0fe16 in js::jit::SpecializedRegSet<js::jit::AllocatableSetAccessors<js::jit::TypedRegisterSet<js::jit::Register> >, js::jit::TypedRegisterSet<js::jit::Register> >::take(js::jit::Register) js/src/jit/RegisterSets.h:742:9
    #1 0x2a0fe16 in js::wasm::BaseCompiler::allocGPR(js::jit::Register) js/src/asmjs/WasmBaselineCompile.cpp:538
    #2 0x29f7e4e in js::wasm::BaseCompiler::needI64(js::wasm::BaseCompiler::RegI64) js/src/asmjs/WasmBaselineCompile.cpp:802:9
    #3 0x29f7e4e in js::wasm::BaseCompiler::popI64(js::wasm::BaseCompiler::RegI64) js/src/asmjs/WasmBaselineCompile.cpp:1303
    #4 0x29fb90c in js::wasm::BaseCompiler::popJoinReg() js/src/asmjs/WasmBaselineCompile.cpp:1464:27
    #5 0x29d4dfa in js::wasm::BaseCompiler::emitBrTable() js/src/asmjs/WasmBaselineCompile.cpp:4698:16
    #6 0x29e793b in js::wasm::BaseCompiler::emitBody() js/src/asmjs/WasmBaselineCompile.cpp:5700:13
    #7 0x29f0745 in js::wasm::BaseCompiler::emitFunction() js/src/asmjs/WasmBaselineCompile.cpp:6215:10
    #8 0x29f47a2 in js::wasm::BaselineCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmBaselineCompile.cpp:6482:10
    #9 0x72c845 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3477:16
    #10 0x6afca0 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:824:14
    #11 0x645e3d in DecodeFunctionBody(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/Wasm.cpp:940:12
    #12 0x645e3d in DecodeCodeSection(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/Wasm.cpp:968
    #13 0x645e3d in DecodeModule(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::wasm::ShareableBytes const&, JS::MutableHandle<js::ArrayBufferObject*>) js/src/asmjs/Wasm.cpp:1141
    #14 0x63bc72 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/Wasm.cpp:1249:27
    #15 0x59a42e in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5226:14
    #16 0x1ea21d1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235:15
[...]
    #29 0x461088 in _start (/home/ubuntu/build/build/js+0x461088)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/jit/RegisterSets.h:742:9 in js::jit::SpecializedRegSet<js::jit::AllocatableSetAccessors<js::jit::TypedRegisterSet<js::jit::Register> >, js::jit::TypedRegisterSet<js::jit::Register> >::take(js::jit::Register)
==12391==ABORTING

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

A simple bug in brTable I think: brTable is careful not to allocate the branch condition value (always I32) in the join register (that is used to carry the value along the control flow edge), but only if the type of the brTable is I32.  But on 64-bit systems the 32-bit and 64-bit integer registers overlap, so it needs to be sure to avoid this even if the brTable type is I64.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1280933-joinreg.patch

Fix + test case

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8764152 [details] [diff] [review]
bug1280933-joinreg.patch

Ignore, will upload with .wast test case

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1280933-joinreg.patch

With text-format test case.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8764161 [details] [diff] [review]
bug1280933-joinreg.patch

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

I understand this fix, but this is kinda sad as we have a smart mechanism for signalling that registers can alias and how they alias, for float registers. This patch is fine as is, but a better way to do it would be to introduce the same kind of masking we have for FP registers. Thank you for the patch, r=me with the test fixed.

::: js/src/jit-test/tests/wasm/regress/reserve-joinreg.js
@@ +1,4 @@
> +// |jit-test| test-also-wasm-baseline
> +load(libdir + "wasm.js");
> +
> +// Bug 1280933, excerpted from binary test case provided there.

For what it's worth, that's what the "random-*.js" were supposed to mean at start (bugs found by fuzzers), but I actually prefer your way of having a regress/ directory.

/me dreams of free time to split tests by operation type, instead of having a few gigantic test files

@@ +23,5 @@
> +      (br_table $1 $2 $3 (i32.const 1) (i64.const 2))))))
> +  (export "" 0))`);
> +
> +} catch (e) {
> +  if (!(e instanceof TypeError && e.message.match(/validation error/)))

Well, I think this breaks the purpose of the test to have a validation error, doesn't it? If it doesn't validate, it won't reach the stage of baseline compilation first. Could you fix up the test enough so that it MOZ_CRASHes without your fix, and doesn't otherwise, instead?

Comment 7

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Comment on attachment 8764161 [details] [diff] [review]
> bug1280933-joinreg.patch
> 
> Review of attachment 8764161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I understand this fix, but this is kinda sad as we have a smart mechanism
> for signalling that registers can alias and how they alias, for float
> registers. This patch is fine as is, but a better way to do it would be to
> introduce the same kind of masking we have for FP registers.

It is likely this code will need to evolve further once we start supporting 64-bit on 32-bit platforms and registers will be allocated in pairs, of which x86 only has three.

> Thank you for
> the patch, r=me with the test fixed.

OK, I'll look into that, though note the test is the one provided by fuzzing, and does not need to pass validation.

Comment 8

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01edfb2991d015726bedbaa5c778869d2d3c4fca
Bug 1280933 - reserve join reg at control flow for i64 results. r=bbouvier

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a277f3e065d2a4026a6c10d2dae02699ef30a504
Bug 1280933 - disable test on 32-bit platforms.  r=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a277f3e065d2
disable test on 32-bit platforms.  r=me

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/01edfb2991d0
https://hg.mozilla.org/mozilla-central/rev/a277f3e065d2