1448552 - Assertion failure: !ra.isScratchRegisterTaken(s), at js/src/wasm/WasmBaselineCompile.cpp:845

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision 05d3bc0ebe47 (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 --enable-simulator=arm64). To reproduce, you can run the following code in the JS shell:

var data = os.file.readFile(file, 'binary');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==30325==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000025e0ec9 bp 0x7ffc54192f90 sp 0x7ffc54192ee0 T0)
    #0 0x25e0ec8 in js::wasm::BaseScratchRegister::BaseScratchRegister(js::wasm::BaseRegAlloc&, js::wasm::BaseRegAlloc::ScratchKind) js/src/wasm/WasmBaselineCompile.cpp:845:9
    #1 0x25e0ec8 in js::wasm::ScratchI32::ScratchI32(js::wasm::BaseRegAlloc&) js/src/wasm/WasmBaselineCompile.cpp:899
    #2 0x25e0ec8 in js::wasm::BaseCompiler::sync() js/src/wasm/WasmBaselineCompile.cpp:2468
    #3 0x251e96e in js::wasm::BaseRegAlloc::needI32() js/src/wasm/WasmBaselineCompile.cpp:711:16
    #4 0x251e96e in js::wasm::BaseCompiler::needI32() js/src/wasm/WasmBaselineCompile.cpp:1942
    #5 0x251e96e in js::wasm::BaseCompiler::emitGetGlobal() js/src/wasm/WasmBaselineCompile.cpp:7986
    #6 0x2531f43 in js::wasm::BaseCompiler::emitBody() js/src/wasm/WasmBaselineCompile.cpp:9032:13
    #7 0x2546b8a in js::wasm::BaseCompiler::emitFunction() js/src/wasm/WasmBaselineCompile.cpp:9704:10
    #8 0x2546b8a in js::wasm::BaselineCompileFunctions(js::wasm::ModuleEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmBaselineCompile.cpp:9856
    #9 0x27b24c7 in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:627:14
    #10 0x27b3199 in js::wasm::ModuleGenerator::launchBatchCompile() js/src/wasm/WasmGenerator.cpp:696:14
    #11 0x27b42c7 in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:772:26
    #12 0x270ad2b in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:84:15
    #13 0x270950c in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:436:10
    #14 0x2819c8e in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:328:27
    #15 0x5d37cb in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:6798:14
[...]

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: scratch.patch

Easy one: ScratchI32 has been hoisted up, but the needI32() / needI64() call can try to reuse the ScratchI32 register in the hidden sync() call, so defer the definition after the need() calls.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8962366 [details] [diff] [review]
scratch.patch

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

Thanks!

Comment 4

[Luke Wagner [:luke]]

Do you now what the regressing cset is (and whether we need to land the fix on a branch)?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Definitely bug 1412238, which added this line and the use of ScratchI32: changeset is 5c82560d19d9, that was landed three days ago (see also bug 1412238 comment 42).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f29b6bcca6
Don't reserve the scratch register before a possible sync in wasm baseline get_global; r=luke

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/31f29b6bcca6

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: setglobal.patch

Sorry, I did miss that set_global was affected too.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb13eb4b2c58
Fix same issue in set_global with the wasm baseline compiler; r=luke

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/bb13eb4b2c58

Comment 11

[Lars T Hansen [:lth]]

I believe I also fixed (or avoided introducing) another case myself.  This really should be amenable to static analysis...

Steve, are we doing any static analysis on ScratchRegisterScope usage to ensure that scopes are not nested?

Comment 12

[Steve Fink [:sfink] [:s:]]

(In reply to Lars T Hansen [:lth] from comment #11)
> Steve, are we doing any static analysis on ScratchRegisterScope usage to
> ensure that scopes are not nested?

We are not. The infrastructure we would need is mostly there, with one exception -- this really wants to be tested across all architectures, because iiuc scratch register usage is arch-dependent. Anything we can cross-compile from gcc should be fairly straightforward, I think?

If you file a bug describing the specific analysis you want, it would make it more likely to happen. (I think I already know, but I also know I'm not going to get to it soon so the more clear it is, the more likely I'll be able to pick it up when I have some time.)

Comment 13

[Lars T Hansen [:lth]]

(In reply to Steve Fink [:sfink] [:s:] from comment #12)
> (In reply to Lars T Hansen [:lth] from comment #11)
> > Steve, are we doing any static analysis on ScratchRegisterScope usage to
> > ensure that scopes are not nested?
> 
> We are not. The infrastructure we would need is mostly there, with one
> exception -- this really wants to be tested across all architectures,
> because iiuc scratch register usage is arch-dependent. Anything we can
> cross-compile from gcc should be fairly straightforward, I think?

I think so.  Effectively compiling with the arm/arm64/mips32/mips64 simulators should be fine, so we should be able to cover all platforms with gcc running on linux64.  MIPS is obviously optional but basically a freebie.

> If you file a bug describing the specific analysis you want, it would make
> it more likely to happen. (I think I already know, but I also know I'm not
> going to get to it soon so the more clear it is, the more likely I'll be
> able to pick it up when I have some time.)

Bug 1451249, there's a surprising amount of gore actually (assuming I even know everything that's going on).