1565768 - Assertion failure: baseline || ion || cranelift, at js/src/wasm/WasmCompile.cpp:130 with asm.js and wonky compiler selection

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P3)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision f1d381735279 (build with --enable-address-sanitizer, run with --fuzzing-safe --no-threads --no-baseline --no-ion --wasm-compiler=cranelift):

(function(x, y, z) {
    "use asm";
    function f() {}
    return f;
})();

Backtrace:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==19582==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x563e12f709d8 bp 0x7fff5f9e6ad0 sp 0x7fff5f9e6a70 T0)
==19582==The signal is caused by a WRITE memory access.
==19582==Hint: address points to the zero page.
    #0 0x563e12f709d7 in js::wasm::CompileArgs::build(JSContext*, js::wasm::ScriptedCaller&&) js/src/wasm/WasmCompile.cpp:130:3
    #1 0x563e13016b8d in ModuleValidator<char16_t>::finish() js/src/wasm/AsmJS.cpp:2124:30
    #2 0x563e12ebda4f in RefPtr<js::wasm::Module const> CheckModule<char16_t>(JSContext*, js::frontend::Parser<js::frontend::FullParseHandler, char16_t>&, js::frontend::ParseNode*, unsigned int*) js/src/wasm/AsmJS.cpp:6413:27
    #3 0x563e12ebda4f in bool DoCompileAsmJS<char16_t>(JSContext*, js::frontend::Parser<js::frontend::FullParseHandler, char16_t>&, js::frontend::ParseNode*, bool*) js/src/wasm/AsmJS.cpp:7084
    #4 0x563e12ebda4f in js::CompileAsmJS(JSContext*, js::frontend::Parser<js::frontend::FullParseHandler, char16_t>&, js::frontend::ParseNode*, bool*) js/src/wasm/AsmJS.cpp:7122
/snip

For detailed crash information, see attachment.

Seems to be a repeat of bug 1558165, but for ASan builds. For now, I'd have to disable cranelift on ASan.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Lars, thoughts? (:bbouvier seems to be out for awhile)

Comment 3

[Lars T Hansen [:lth]]

Cranelift doesn't support asm.js and there's probably a guard missing somewhere. I'm surprised that asan is needed here. I'll look into it.

Comment 4

[Lars T Hansen [:lth]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)

For now, I'd have to disable cranelift on ASan.

That doesn't make any sense, BTW, since this is asm.js content and cranelift does not support asm.js. The assertion you see here is that you've selected a compiler that is not supported for the content you're processing. The bug is likely that enabling asan disables some check somewhere that should have given you an error message earlier.

Comment 5

[Lars T Hansen [:lth]]

Gary, how do you configure this build? When I configure with --enable-address-sanitizer and my usual --enable-debug --disable-optimize --without-intl-api I get a lot of linking errors for duplicate symbols in cxxalloc, eg,

ld.lld: error: duplicate symbol: operator delete[](void*)
>>> defined at asan_new_delete.cc:170 (/builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:170)
>>>            asan_new_delete.cc.o:(operator delete[](void*)) in archive /home/lhansen/.mozbuild/clang/lib/clang/8.0.0/lib/linux/libclang_rt.asan_cxx-x86_64.a
>>> defined at cxxalloc.h:59 (/home/lhansen/m-i/js/src/build-asan/dist/include/mozilla/cxxalloc.h:59)
>>>            ../../../memory/mozalloc/cxxalloc.o:(.text+0xC0)

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Sorry, I was missing --disable-cranelift.

Edit: Fails
AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --enable-address-sanitizer --disable-jemalloc --disable-tests --disable-cranelift


Edit: Works
AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --enable-address-sanitizer --disable-jemalloc --disable-tests

on Ubuntu 18.04 with the following compiler:

$ clang --version
clang version 7.0.0-3~ubuntu0.18.04.1 (tags/RELEASE_700/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ clang++ --version
clang version 7.0.0-3~ubuntu0.18.04.1 (tags/RELEASE_700/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

I guess it boils down to the question: should I compile ASan js shell with cranelift enabled or disabled? I remember I shut it off as I would get compile errors some of the time, but I'm not sure I had a canonical answer as to turn it on or off.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

fwiw, I have re-enabled cranelift only on recent m-c builds (likely rev 9e7c1e1a993d and later), so going forward, I don't think we need the --disable-cranelift flag. This affects bisection though, which hopefully I've worked around by only adding the flag in various broken ranges [1]. If so, I can resolve this issue.

[1] added it for ASan builds on/after rev 6fcf54117a3b and before 9e7c1e1a993d; added it for non-ASan builds on/after rev 4d9500ca5761 and before 9e7c1e1a993d

Comment 8

[Lars T Hansen [:lth]]

Comment 6 gets it backward, it fails with --disable-cranelift and works if cranelift is enabled (without --disable-cranelift).

I don't think asan or the fuzzing args has anything to do with anything. If I configure with only --disable-cranelift and run with --wasm-compiler=cranelift --no-baseline --no-ion it segfaults. If I configure with --disable-cranelift --enable-debug and run with the same arguments I get the expected assert. This is true also for a more recent rev.

Comment 9

[Lars T Hansen [:lth]]

The guard in EstablishPreconditions (in AsmJS.cpp) is wrong. It tests HasCompilerSupport() && IonCanCompile(), but HasCompilerSupport is wrong because it returns true if any compiler has been compiled-in (so it does not return false just because cranelift is not present) and IonCanCompile returns true if ion could in principle compile the code, regardless of whether it's enabled or not.

Comment 10

[Lars T Hansen [:lth]]

Attachment: Bug 1565768 - asm.js optimized compilation must gate on ion-for-wasm being available. r?jseward

asm.js optimized compilation can be disabled by the unavailability of
ion-for-wasm through command line option selection; we must therefore
gate carefully.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Lars T Hansen [:lth] from comment #8)

Comment 6 gets it backward, it fails with --disable-cranelift and works if cranelift is enabled (without --disable-cranelift).

D'oh, you're right, I got it backwards. I'll edit the comment. Thanks for checking!

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Gary is this something you might check in while Lars is out?

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #12)

Gary is this something you might check in while Lars is out?

I don't think I have the permissions for it in Phabricator?

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Landing to help moving forward here.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b01f6dc63ca
asm.js optimized compilation must gate on ion-for-wasm being available. r=jseward

Comment 16

[Tom S [:evilpie]]

Shouldn't this still check cx->options().asmJS()?

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6b01f6dc63ca

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Tom Schuster [:evilpie] from comment #16)

Shouldn't this still check cx->options().asmJS()?

Setting needinfo? to Benjamin for Tom's comment.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks Gary.

Shouldn't this still check cx->options().asmJS()?

It does so in IsAsmJSCompilationAvailable, where this was done before. EstablishPreconditions uses parser.options.asmjsoption which is inferred from cx->options().asmJS() too. So that's fine as is.