Crashing on this while testing Firefox for Android.

Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 10792] 0x502a2c4c in js::jit::AllocationIntegrityState::checkIntegrity (this=0xd3, this@entry=0x5437f450, block=block@entry=0x0, ins=<optimized out>, vreg=vreg@entry=34, alloc=..., populateSafepoints=populateSafepoints@entry=false) at js/src/jit/RegisterAllocator.cpp:206 206 JS_ASSERT(*def->output() == alloc); (gdb) back #0 0x502a2c4c in js::jit::AllocationIntegrityState::checkIntegrity (this=0xd3, this@entry=0x5437f450, block=block@entry=0x0, ins=<optimized out>, vreg=vreg@entry=34, alloc=..., populateSafepoints=populateSafepoints@entry=false) at js/src/jit/RegisterAllocator.cpp:206 #1 0x502a380c in js::jit::AllocationIntegrityState::check (this=0x5437f450, populateSafepoints=<optimized out>) at js/src/jit/RegisterAllocator.cpp:162 #2 0x5011f45c in js::jit::GenerateLIR (mir=mir@entry=0x5df731f0) at js/src/jit/Ion.cpp:1616 #3 0x5015cdcc in js::jit::CompileBackEnd (mir=0x5df731f0) at js/src/jit/Ion.cpp:1700 #4 0x504c59e0 in js::HelperThread::handleIonWorkload (this=this@entry=0x4560c4e0) at js/src/vm/HelperThreads.cpp:967 #5 0x504c6c30 in js::HelperThread::threadLoop (this=0x4560c4e0) at js/src/vm/HelperThreads.cpp:1266

Reproduces in the ARM Simulator, but it took some time.

The failure occurs when the JIT is compiling JS to Ion code, and thus when using the LSRA register allocator. Switching to the Backtracking register allocator avoids a failure so it might be specific to the LSRA register allocator. Here's the reg alloc spew which gives some clues: v34[0] req(r) has(s16) [76,108) v34:r@108 ... v49[0] req(r) has(s29) [106,108) / v49[1] req(r@404?) has(s16) [108,110) / v49[2] req(r@404?) has(s0) [110,112) / v49[3] req(r@404?) has(stack:20) [112,403) / v49[4] req(r) has(s16) [403,404) v49:r@404 ... [76,77 LoadTypedArrayElement] [def v34<f>:s16] [use v33:r r2] [use c] [78,79 LoadTypedArrayElement] [def v35<f>:s17] [use v33:r r2] [use c] [80,81 LoadTypedArrayElement] [def v36<f>:s18] [use v33:r r2] [use c] [82,83 LoadTypedArrayElement] [def v37<f>:s19] [use v33:r r2] [use c] [84,85 LoadTypedArrayElement] [def v38<f>:s20] [use v33:r r2] [use c] [86,87 LoadTypedArrayElement] [def v39<f>:s21] [use v33:r r2] [use c] [88,89 LoadTypedArrayElement] [def v40<f>:s22] [use v33:r r2] [use c] [90,91 LoadTypedArrayElement] [def v41<f>:s23] [use v33:r r2] [use c] [92,93 LoadTypedArrayElement] [def v42<f>:s24] [use v33:r r2] [use c] [94,95 LoadTypedArrayElement] [def v43<f>:s25] [use v33:r r2] [use c] [96,97 LoadTypedArrayElement] [def v44<f>:s26] [use v33:r r2] [use c] [98,99 LoadTypedArrayElement] [def v45<f>:s27] [use v33:r r2] [use c] [100,101 LoadTypedArrayElement] [def v46<f>:s28] [use v33:r r2] [use c] [102,103 LoadTypedArrayElement] [def v47<f>:s29] [use v33:r r2] [use c] [MoveGroup] [s29 -> stack:4] [104,105 LoadTypedArrayElement] [def v48<f>:s29] [use v33:r r2] [use c] [MoveGroup] [s29 -> stack:8] [106,107 LoadTypedArrayElement] [def v49<f>:s29] [use v33:r r2] [use c] [MoveGroup] [s29 -> s16] [s28 -> stack:12] [108,109 Float32ToDouble] [def v50<d>:d14] [use v34:r s16] [MoveGroup] [s16 -> s0] [s27 -> stack:16] [s17 -> s27] [110,111 Float32ToDouble] [def v51<d>:d8] [use v15:r s0] [MoveGroup] [s0 -> stack:20] [s26 -> stack:24] [s1 -> s26] Note that s16 is loaded at 76,77 and is overwritten by the MoveGroup before 108,109 and then used by 108,109. The assertion failure occurs when checking vreg v34, the vreg argument to checkIntegrity() is 34. Just before the failure the code checks 'info.outputs[i].virtualRegister() == vreg' where the lhs is 49 and the rhs vreg is 34. It might be related to the recent float32 improvements, bug 991153. mjrosenb: Does the above give an clue to the problem? The function that fails to compile is not too large, and perhaps if exercised appropriately then a reproducible test can be developed, so I'll give this a try.

Here is a simple test case that reproduces the crash.

My first instinct is that the generated code is fine, but the integrity checker doesn't know how to handle the aliased registers (I forgot the integrity checker existed, and didn't want to write something like this that could handle the aliases, which is why I use the simulator in the test function)

(In reply to Marty Rosenberg [:mjrosenb] from comment #5) > My first instinct is that the generated code is fine, but the integrity > checker doesn't know how to handle the aliased registers (I forgot the > integrity checker existed, and didn't want to write something like this that > could handle the aliases, which is why I use the simulator in the test > function) Could you take another look at comment 3, in particular: [106,107 LoadTypedArrayElement] [def v49<f>:s29] [use v33:r r2] [use c] [MoveGroup] [s29 -> s16] [s28 -> stack:12] [108,109 Float32ToDouble] [def v50<d>:d14] [use v34:r s16] It does look like the MoveGroup overwrites s16 destroying v34 before it is used? Let me see if a numerical error can be found. and inspect the generated machine code.

[Tracking Requested - why for this release]: This seems to break WMW.

Nominating for tracking 33 assuming bug 991153 is what broke this.

Marty can you take this? It's broken on Aurora too so we should fix it there as well. Thanks!

ok, as usual, the register allocator has lots of sharp corners, and I'm not entirely sure which ones are supposed to be covered. v16, v19, and v40 are all float32 loads. v46 is converting v19 to a double. We are then given the intervals: v19[0] [38,92) v19:r@92 v40[0] [80,212) v40:r@212 v46[0] [92,209) v46:r@95 v46:r@209 v19 gets assigned to register s4 a while later: v40 gets assigned s22 shortly after that, we start processing v46, at position 92. v19 finishes its interval, all double registers are occupied (including d2, since s5 is still active), so s22 is chosen to be evicted, and v46 is assigned to d11. part of being evicted: Split interval to 40 = [80, 92]/[92, 212] now, we process the new interval, starting at 92: since s4 is now free v40[1] is assigned to s4. at this point, we have ensured that s4 won't have the proper value of v19 by the time instruction 46 gets around to executing. I suspect that this hasn't come up before because the float32ToDouble would simply re-use its input, and wouldn't overwrite the register until the output half of the instruction.

Since I'm tired, and my previous explanation was a bit unwieldy, here is a more concise explanation Float32ToDouble's only input is marked as useAtStart, so you can generate cvtf2d xmm0, xmm0, or cvt.f64.f32, d2, s4. there is an instruction v46 <- Float32ToDouble(v19), where v19 currently resides in s4. Something else, doesn't really matter what lives in s5, so d2 is unavailable. d11 is selected as the residence for v46, which evicts v40 from its residence in s22. Normally, when we go to find a new residence for v40, nothing would be found, and it would be placed on the stack. In this instance, however, s4 is free, so it is selected.

Not an elegant solution, but effective.

hey, dan, twofer. First, is it possible that the backtracking allocator is susceptible to this kind of corner case (described in comment 10 and comment 11). Secondly, is something like this the reason that lowerForFPU only has useRegisterAtStart for its lhs, but not usually its rhs?

(In reply to Marty Rosenberg [:mjrosenb] from comment #11) > Since I'm tired, and my previous explanation was a bit unwieldy, here is a > more concise explanation > Float32ToDouble's only input is marked as useAtStart, so you can generate > cvtf2d xmm0, xmm0, or cvt.f64.f32, d2, s4. > there is an instruction v46 <- Float32ToDouble(v19), where v19 currently > resides in s4. Something else, doesn't really matter what lives in s5, so > d2 is unavailable. d11 is selected as the residence for v46, which evicts > v40 from its residence in s22. Normally, when we go to find a new residence > for v40, nothing would be found, and it would be placed on the stack. In > this instance, however, s4 is free, so it is selected. Why does s4 appear free for v40? We haven't evicted v19 or anything, so it's in use for v9 still, right?

because the lifetime of v19 ends right before its use in v46. This happens because it is useRegisterAtStart, rather than useRegister. Moreover, as long as we want v46 able to be allocated to the same register as v19, wherever we split v40, it'll be able to see a free register. If it didn't then v46 wouldn't be able to go into the same register.

Oh, then this may also be affected by LinearScan's interpretation of live ranges. This: v19[0] [38,92) v19:r@92 seems to say that the live range ends before 92 (since it's exclusive at the to side), and yet the live interval still has a use at 92. Liveness for the backtracking allocator is computed such that uses are contained within live ranges. The live range for v19 would be [38,93) since the use is at 92, and the live range for v46 would be [93,209) since the def happens on the output side of the instruction. With those ranges, v19 doesn't conflict with v46, and there's no window between them where v40 can see the register as free.

Crash. Tracking.

Comment on attachment 8463951 [details] [diff] [review] ConservativeRegisters-r0.patch Review of attachment 8463951 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay! There's a ton of debugging code in this patch though, a |hg qdiff| or something before uploading a patch is a great way to catch such things. ::: js/src/jit/LIR.cpp @@ +545,5 @@ > + if (from->isFloatReg() && from->toFloatReg()->reg().code() == 22 && > + to->isFloatReg() && to->toFloatReg()->reg().code() == 4) { > + fprintf(stderr, "JACKPOT!\n"); > + MOZ_CRASH("boom"); > + } Remove. ::: js/src/jit/LinearScan.cpp @@ +109,5 @@ > > if (it->end() <= position) { > + IonSpew(IonSpew_RegAlloc, "finishing v%d = [%u, %u]", > + it->hasVreg() ? it->vreg() : 0, it->start().bits(), it->end().bits()); > + Remove this line (has trailing whitespace too). @@ +711,5 @@ > for (IntervalIterator i(active.begin()); i != active.end();) { > if (i->getAllocation()->isRegister() && > i->getAllocation()->toRegister().aliases(allocatedReg)) > { > + AnyRegister ireg = i->getAllocation()->toRegister(); This variable is unused. @@ +717,1 @@ > vregs[i->vreg()].ins()->id(), i->start().bits(), i->end().bits()); Revert the "v%u =" change, it's not a virtual register but an instruction id. We could change that but that's better done in a follow-up patch. @@ +727,5 @@ > finishInterval(it); > if (allocatedReg.numAliased() == 1) > break; > } else { > + IonSpew(IonSpew_RegAlloc, " Not touching active interval v%u = [%u, %u]", Same here and below. ::: js/src/jit/Lowering.cpp @@ +1756,5 @@ > } > > case MIRType_Float32: > { > + LFloat32ToDouble *lir = new(alloc()) LFloat32ToDouble(useRegister(opd)); Can you assert in useRegisterAtStart that the argument's type != MIRType_Float32, to avoid introducing similar bugs elsewhere? Else we should add a comment here to avoid reintroducing the bug. ::: js/src/jit/RegisterAllocator.cpp @@ +176,5 @@ > AllocationIntegrityState::checkIntegrity(LBlock *block, LInstruction *ins, > uint32_t vreg, LAllocation alloc, bool populateSafepoints) > { > + int orig_id = ins->id(); > + LAllocation orig_alloc = alloc; Remove. @@ +181,3 @@ > for (LInstructionReverseIterator iter(block->rbegin(ins)); iter != block->rend(); iter++) { > ins = *iter; > + fprintf(stderr, "processing: inst %d\n", ins->id()); Remove. @@ +188,5 @@ > LMoveGroup *group = ins->toMoveGroup(); > for (int i = group->numMoves() - 1; i >= 0; i--) { > if (*group->getMove(i).to() == alloc) { > + fprintf(stderr, "following movegroup: %s ", alloc.toString()); > + fprintf(stderr, "-> %s\n", group->getMove(i).from()->toString()); Remove.

Marty, any reason why this didn't land in m-c yet? Thanks

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9397eddf0862

emergency follow-up because win8 decided to explode for no reason that has been discerned thusfar: https://hg.mozilla.org/integration/mozilla-inbound/rev/7eeb76564dde on second thought, something like this is probably useful in the long run, because we do actually want this behavior on non-arm platforms.

The failures on inbound did not give many clues, the asm.js testSIMD.js failed, but it's not clear why, and I can not reproduce it locally on Linux. Here's try run on m-c: https://tbpl.mozilla.org/?tree=Try&rev=85c86274cdc8

cc'ing :bbouvier because he believes this is an alignment issue, and may be able to offer some insight.

ok, I managed to catch this guy: 0: a1 10 16 97 04 mov 0x4971610,%eax 5: ff 70 34 pushl 0x34(%eax) 8: 89 60 34 mov %esp,0x34(%eax) b: 83 ec 10 sub $0x10,%esp e: e9 08 00 00 00 jmp 0x1b 13: f4 hlt 14: f4 hlt 15: f4 hlt 16: f4 hlt 17: f4 hlt 18: 83 ec 14 sub $0x14,%esp 1b: 0f 28 05 00 16 97 04 movaps 0x4971600,%xmm0 22: f3 0f 10 0d f0 15 97 movss 0x49715f0,%xmm1 29: 04 2a: f3 0f 5a c9 cvtss2sd %xmm1,%xmm1 2e: f2 0f 10 15 d8 15 97 movsd 0x49715d8,%xmm2 35: 04 36: 66 0f 2e ca ucomisd %xmm2,%xmm1 3a: 0f 83 01 00 00 00 jae 0x41 40: cc int3 41: f2 0f 10 15 e0 15 97 movsd 0x49715e0,%xmm2 48: 04 49: 66 0f 2e d1 ucomisd %xmm1,%xmm2 4d: 0f 83 01 00 00 00 jae 0x54 53: cc int3 54: f2 0f 5a c9 cvtsd2ss %xmm1,%xmm1 58: 83 ec 10 sub $0x10,%esp > 5b: 0f 29 04 24 movaps %xmm0,(%esp) 5f: f3 0f 11 4c 24 04 movss %xmm1,0x4(%esp) 65: 0f 28 04 24 movaps (%esp),%xmm0 69: 83 c4 10 add $0x10,%esp 6c: 0f 2e c0 ucomiss %xmm0,%xmm0 6f: 0f 8b 08 00 00 00 jnp 0x7d 75: f3 0f 10 05 f4 15 97 movss 0x49715f4,%xmm0 7c: 04 7d: f3 0f 5a c8 cvtss2sd %xmm0,%xmm1 81: f2 0f 11 0c 24 movsd %xmm1,(%esp) 86: 66 0f 28 c1 movapd %xmm1,%xmm0 8a: 66 90 xchg %ax,%ax 8c: 83 c4 14 add $0x14,%esp 8f: c3 ret 90: 83 c4 10 add $0x10,%esp 93: 8b 0d 10 16 97 04 mov 0x4971610,%ecx 99: 8f 41 34 popl 0x34(%ecx) and as expected, esp is: 0x0033e9c8, which is not 16-byte aligned. It looks like this is the codegen for SimdInsertElementF.

The stack alignment is wrong in the function in comment 24. The stack appears to have been correctly aligned before the call, but after pushing the return address, and the 0x14 frame reservation, the stack is no longer aligned to 16 bytes. The frame depth might need to be padded for the SIMD stack alignment even if there are no function calls because some sequences push values onto the stack and read back SIMD sized values that need to be aligned. Lets give it a try: https://tbpl.mozilla.org/?tree=Try&rev=0e51bca54c42

Oh crud, that was a silly bug on my part. Thanks for finding and fixing you guys.

Also, this has pointed out that AsmJSStackAlignment isn't quite the right name given that (with dougc's try patch), sp is only word-aligned on non-simd-containing leaf functions. AsmJSStackAlignmentIfCallOrSimd?

https://hg.mozilla.org/mozilla-central/rev/9397eddf0862 https://hg.mozilla.org/mozilla-central/rev/7eeb76564dde

Comment on attachment 8494523 [details] [diff] [review] Always pad the stack to the SIMD alignment. Looks like this addressed the issue, based on the results of the try run in comment 25. If you would like AsmJSStackAlignment renamed then just let me know. bbouvier suggested that the instruction sequences should be using the existing stack frame rather than allocating stack dynamically, and this would be a good longer term solution.

Comment on attachment 8494523 [details] [diff] [review] Always pad the stack to the SIMD alignment. Review of attachment 8494523 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! rs=me if you want to s/AsmJSStackAlignment/AsmJSStackAlignmentIfCallOrSimd/ (or a better name you can think of). It'd also be nice to update the AsmJSFrame comment in Assembler-shared.h to reflect at what points the property is true. ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +84,3 @@ > frameDepth_ += frameInitialAdjustment_; > + // Keep the stack aligned. Some SIMD sequences build values on the > + // stack and need the stack aligned. Can you move the "If the function uses any SIMD" comment into the then-block and put a \n before the "Keep the stack aligned." comment?

Thank you for the quick review. Address reviewer feedback, but have not renamed AsmJSStackAlignment which seems ok to me. Carrying forward r+.

Reopened to land the stack alignment patch. Probably should have created a separate bug for it, but this issue started here.

Do you have a Try link handy for this? :)

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33) > Do you have a Try link handy for this? :) Yes, there was a try run in comment 25. The only change was to move a comment.

Sorry, missed it amongst the comments related to the other patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/ea64366d660f

Douglas, can we have an uplift request for aurora & beta? Thanks

Here's that actual patch that landed on m-c, taken from the hg changeset. Carrying forward the r+.

Plus a fix for issues tickled on Windows 8. Taken from the hg changeset landed on m-c. Carrying forward the r+.

Try run on Aurora 34 of patches 1.1, 1.2, and 2: https://tbpl.mozilla.org/?tree=Try&rev=7a4c36ccb9d3 Try run on Beta 33 of patches 1.1, and 1.2 (patch 2 is not applicable to Beta 33): https://tbpl.mozilla.org/?tree=Try&rev=feb0f60eab5a

Comment on attachment 8495162 [details] [diff] [review] 2. Pad the stack to the SIMD alignment if there are calls or SIMD instructions. Approval Request Comment [Feature/regressing bug #]: 992267 [User impact if declined]: Crashes in SIMD code, caused by unaligned stack loads or stores. [Describe test coverage new/current, TBPL]: It fixed test failures identified when patch 1.1 alone landed on m-c. It has landed on m-c. [Risks and why]: Low because it fixes an obvious problem and SIMD is not widely used. [String/UUID change made/needed]: n/a

Comment on attachment 8496824 [details] [diff] [review] 1.1 Don't try to re-use the input on float32 -> double conversions, it can go wrong on ARM due to deep-seated reasons. Approval Request Comment [Feature/regressing bug #]: Seems to have been introduced by ARM float32 development. [User impact if declined]: Bad code generation. Corrupted output was noted on a game being worked on with a partner. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Medium. It works around an issue rather than fixing the cause. We are hoping it does not tickle other issues. [String/UUID change made/needed]: n/a

Comment on attachment 8496826 [details] [diff] [review] 1.2 Win8 mysteriously fails with this patch. Fix with an #ifdef until hardware can be acquired CLOSED TREE r=red This was a follow up to patch 1.1, and workaround for crashes on Windows 8. Approval Request Comment [Feature/regressing bug #]: Patch 1.1 in this bug. [User impact if declined]: Crashes on Windows 8. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low as it just reverts a change on unaffected platforms. [String/UUID change made/needed]: n/a

Comment on attachment 8496830 [details] [diff] [review] 1.1 Don't try to re-use the input on float32 -> double conversions, it can go wrong on ARM due to deep-seated reasons. Rebased for Beta 33. Approval Request Comment [Feature/regressing bug #]: Seems to have been introduced by ARM float32 development. [User impact if declined]: Bad code generation. Corrupted output was noted on a game being worked on with a partner. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Medium. It works around an issue rather than fixing the cause. We are hoping it does not tickle other issues. [String/UUID change made/needed]: n/a

Comment on attachment 8496831 [details] [diff] [review] 1.2 Win8 mysteriously fails with this patch. Fix with an #ifdef until hardware can be acquired. Rebased for Beta 33. This was a follow up to patch 1.1, and workaround for crashes on Windows 8. Approval Request Comment [Feature/regressing bug #]: Patch 1.1 in this bug. [User impact if declined]: Crashes on Windows 8. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low as it just reverts a change on unaffected platforms. [String/UUID change made/needed]: n/a

Comment on attachment 8495162 [details] [diff] [review] 2. Pad the stack to the SIMD alignment if there are calls or SIMD instructions. Aurora+

Comment on attachment 8496826 [details] [diff] [review] 1.2 Win8 mysteriously fails with this patch. Fix with an #ifdef until hardware can be acquired CLOSED TREE r=red dougc - Can you please file a follow up to investigate why this workaround is needed?

I just folded the Win8 fix in with the original patch (as I usually do for follow-up bustage fixes on uplifts). https://hg.mozilla.org/releases/mozilla-aurora/rev/2e47a42abc12 https://hg.mozilla.org/releases/mozilla-aurora/rev/2698c6c0881c

(In reply to Lawrence Mandel [:lmandel] from comment #49) > Comment on attachment 8496826 [details] [diff] [review] > 1.2 Win8 mysteriously fails with this patch. Fix with an #ifdef until > hardware can be acquired CLOSED TREE r=red > > dougc - Can you please file a follow up to investigate why this workaround > is needed? It was a stack alignment issue, fixed by patch 2 in this bug. Beta 33 would not have been affected. I still suggest landing patch 1.2, it just backs out the change for Windows.

Should this be considered for b2g 2.0 (b2g32) uplift as well?

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/3819402e564a/js/src/jit/arm/Architecture-arm.h#l202 Looks like the float32 work is not on b2g 2.0, so this fix is unneeded. Unless, of course, you also want to uplift those patches.