1357048 - Hit MOZ_CRASH(ARM simulator breakpoint) at js/src/jit/arm/Simulator-arm.cpp:3226 with asm.js

Status: RESOLVED DUPLICATE of bug 1352506

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ce69b6e1773e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --ion-offthread-compile=off --ion-check-range-analysis --arm-hwcap=vfp):

function m() {
    "use asm";
    function f() {
        var x = 0;
        var y = 0;
        x = (((0x77777777 - 0xcccccccc) | 0) % -1) | 1;
        y = (((0x7FFFFFFF + 0x7FFFFFFF) | 0) % -1) | 0;
        return (x + y) | 0;
    }
    return f;
}
assertEq(m()(), 6 && (this) && (this) && (this))


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x08513838 in js::jit::Simulator::decodeType01 (this=0xf796f000, instr=0x201831ac) at js/src/jit/arm/Simulator-arm.cpp:3226
#0  0x08513838 in js::jit::Simulator::decodeType01 (this=0xf796f000, instr=0x201831ac) at js/src/jit/arm/Simulator-arm.cpp:3226
#1  0x0851032a in js::jit::Simulator::instructionDecode (this=0xf796f000, instr=0x201831ac) at js/src/jit/arm/Simulator-arm.cpp:4728
#2  0x08513e9a in js::jit::Simulator::execute<false> (this=0xf796f000) at js/src/jit/arm/Simulator-arm.cpp:4801
#3  js::jit::Simulator::callInternal (this=0xf796f000, entry=0x20183218 "\004\340-\345\360\037-\351\020\212", <incomplete sequence \355>) at js/src/jit/arm/Simulator-arm.cpp:4886
#4  0x08514141 in js::jit::Simulator::call (this=<optimized out>, entry=0x20183218 "\004\340-\345\360\037-\351\020\212", <incomplete sequence \355>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:4969
#5  0x08930ec0 in js::wasm::Instance::callExport (this=0xf51984c0, cx=0xf791d000, funcIndex=4097, args=...) at js/src/wasm/WasmInstance.cpp:784
#6  0x08931918 in WasmCall (cx=0xf791d000, argc=0, vp=0xf5069068) at js/src/wasm/WasmJS.cpp:1114
#7  0x0817a7e6 in js::CallJSNative (cx=0xf791d000, native=0x8931870 <WasmCall(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291
[...]
eax	0x0	0
ebx	0x8cf1ff4	147791860
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0x8cf1ff4	147791860
edi	0xf7da3df8	-136692232
ebp	0xffffbd38	4294950200
esp	0xffffbce0	4294950112
eip	0x8513838 <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5112>
=> 0x8513838 <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5112>:	movl   $0x0,0x0
   0x8513842 <js::jit::Simulator::decodeType01(js::jit::SimInstruction*)+5122>:	ud2

Comment 1

[Luke Wagner [:luke]]

(Unfortunately independent of bug 1357053)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Could still repro. Pretty sure this is because of bug 1340219.

It's an ARM softdiv/mod-only bug. I think the softmod builtin thunk clobbers r4 (ABINonArgReg0) in this case, and when it's read thereafter, it's incorrect. Regalloc should really preserve registers around softmod/softdiv. I think it's implemented this way currently because softmodi wants its return value to be in r1, and the LIR allocation system doesn't know how to do this for calls. Since it's already a slow fallback, I think we could just do the move at the end of the instruction's codegen.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.deadcode.patch

Some dead code in the register allocator.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.softmod-div.patch

Jon, setting you as reviewer since you've written this code in the first place.

Before this patch, soft mod/div/umodordiv were doing custom register allocation with temporaries to prevent register clobbering. This is now not enough, because wasm doesn't call into builtins directly nowadays; instead, wasm calls a thunk that sets up a frame (for profiling purposes) and then calls into this builtin. More registers can be clobbered on the thunk path, as a matter of fact.

Using the usual LIR call infra for soft calls results in simpler lowering. Since the LIR assumes specific registers for call output (r0 on ARM), we manually do a move from r1 to r0 in the codegen for the modulo operations (which put the result in r1). Since these are soft calls, expected to be slow, I think this is acceptable.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

I've made a try-build, but this is hard to be 100% sure the patch is correct: arm vfp-only is already failing a lot on the wasm test suite, before this patch. The best I could do is ensure I hadn't introduced new failures. We should do something about arm vfp-only.

Note that the Android devices being tested on treeherder don't have idiv, as far as i recall, so they actually use soft-div/mod/umod. jsreftests are run on Android and sometimes generate Ion code for mod/div, so this is tested in practice.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6256543e1ea3c2968056a437e7b8018748fa5713

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8861956 [details] [diff] [review]
2.softmod-div.patch

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

This looks fine to me, but I'm not really the right person to review this.  I did write the hardware div/mod operations for ARM but not the soft div/mod (and it was a long time ago).  Since you say it's hard to be 100% sure from the tests, I think you should ask someone who knows more about this.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8861956 [details] [diff] [review]
2.softmod-div.patch

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

Alright, thank you for your input though :)

Redirecting to sunfish, since this mostly touches LIR and register allocation. For what it's worth, the entire asm.js test suite passes even in VFP-only mode, and no new failures have been introduced when testing wasm.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8861956 [details] [diff] [review]
2.softmod-div.patch

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

Oh my, I somehow forgot that I've done this work in bug 1352506 already. The patch there is even better, so I'll keep the test case from this bug and move the other patch there.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8861952 [details] [diff] [review]
1.deadcode.patch

Moved to the other bug.