1559099 - Cranelift: Segfault crash

Status: RESOLVED FIXED

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

Description

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

Attachment: tests_3bedd39092e675a2c00f8232ff1f5600d3c4f0d2.zip

The following testcase crashes on mozilla-central revision 1cd7d9c0a6c3 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion --wasm-compiler=cranelift w10-out.wrapper w10-out.wasm):

See attachment.

Backtrace:

Thread 1 "js-dbg-64-dm-li" received signal SIGILL, Illegal instruction.
0x0000389b3f0ef641 in ?? ()
(gdb) bt
#0 0x0000389b3f0ef641 in ?? ()
#1 0x00007fffffffb790 in ?? ()
#2 0x0000000000000000 in ?? ()
(gdb)

For detailed crash information, see attachment.

Setting s-s as a start because I'm not sure how bad this SIGILL crash is.

Comment 1

[Christian Holler (:decoder)]

Gary, did this ever reproduce for you outside of GDB? I can reproduce it in GDB, but I can continue. WebAssembly uses SIGILL for traps, in that case it could not be a bug.

Comment 2

[Christian Holler (:decoder)]

Actually, after running for a long time I got a segfault outside of GDB. I will try to get a correct backtrace.

Comment 3

[Christian Holler (:decoder)]

[fuzz-exec] calling $func_4519
[LoggingExternalInterface logging i32.const -1236489146]

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
0x000005511644f7bc in ?? ()
(gdb) c
Continuing.

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
0x000005511644f7bc in ?? ()
(gdb) bt
#0  0x000005511644f7bc in ?? ()
#1  0x0000000000000000 in ?? ()
(gdb) x /i $pc
=> 0x5511644f7bc:       movslq (%rax,%rcx,4),%rcx
(gdb) info reg rax rcx
rax            0x5511644fa68    5845824109160
rcx            0xfffffff2       4294967282
(gdb) c
Continuing.

Program terminated with signal SIGSEGV, Segmentation fault.
The program no longer exists.
(gdb)

Comment 4

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/17ee66cc26ef
user: Mike Hommey
date: Wed Feb 06 03:05:52 2019 +0000
summary: Bug 1522609 - Pass compilers and flags down to cargo on sanitizer/fuzzying/ccov builds. r=ted

Totally unsure if this is correct. Might be something related to cranelift/rust component compilation that's throwing it off. Setting needinfo? from Benjamin and Lars as a start.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1559099: Throw when forcing Cranelift usage on an unsupported platform; r?lth

Comment 6

[Phabricator Automation]

Comment on attachment 9072509 [details]
Bug 1559099: Throw when forcing Cranelift usage on an unsupported platform; r?lth

Revision D35178 was moved to bug 1558165. Setting attachment 9072509 [details] to obsolete.

Comment 7

[Lars T Hansen [:lth]]

This is not urgent, as Cranelift is not in any product (it's in Nightly by opt-in only), but it would be good to make sure we're not putting partners at risk. Will attempt to look into this before long.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Regressed bug is probably incorrect; I suspect Cranelift just never generated the right code for this.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wrapper.js

Note: the following JS file does run into the same crash, but only calls one wasm function. I wonder if we could use the wasm-bindgen (esp. the wasm-gc tool) to remove all functions that aren't transitively called by this wasm function, to make compilation faster.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Smaller wat test case

A smaller wat test case, extracted from the big wasm thanks to binaryen. Unfortunately, it seems that we need binaryen's wasm-as to make it a correct wasm file, because Spidermonkey's wasmTextToBinary creates an invalid binary (the error is that we're trying to pop an empty stack at some point; that's something else to investigate).

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Found it: TLDR is that LICM isn't aware that jump_table_entry can load data, thus it would hoist it above a loop before the out-of-bounds check (for the jump table) was done. Patch incoming.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

PR is in https://github.com/CraneStation/cranelift/pull/805 and will need a review. I'll add the Cranelift bump here, as well as supplementary code needed to not panic in our bindings.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1559099: Bump Cranelift in Spidermonkey to e455f6ae; r=lth

Changes have been reviewed on the Cranelift side; this is just a bump to the
latest version.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1559099: Don't check metadata for JumpTableEntry instructions; r=lth

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

This has landed on inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c4ed2de25e6db29bc346f82b8a198c5b4a533b22

b684d4532b91 Bug 1559099: Don't check metadata for JumpTableEntry instructions; r=lth
e73151bcaff8 Bug 1559099: Bump Cranelift in Spidermonkey to e455f6ae; r=lth

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e73151bcaff8
https://hg.mozilla.org/mozilla-central/rev/b684d4532b91