The problem is that we rely on being able to read the frame descriptor, but exit frames remove stack above the return address, stripping away this critical piece of information.

It seems like we have no choice but to detect invalidation inside VM wrappers. Another option would be dropping the stack in callVM rather than generateVMWrapper. Either one seems fine, but I'll try the former first. The idea is to push the return address and see if it's changed on the way out.

This patch makes VM wrappers save their return address before calling, and it's compared against the actual return address on the way out. If they don't match, we avoid using "retn", which would remove the frame descriptor needed by the invalidation thunk.

Comment on attachment 584896 [details] [diff] [review] part 1: fix crashes Marty, could you check the ARM changes?

This patch fixes two additional bugs on this test case: (1) When invalidating, we might have extra bytes on the stack, which messes up snapshots that have captured a distance from sp to fp. So instead, now we snapshot offsets from fp instead. (2) jsinterp had a small bug that tried to unwind extra frames after OSR. (3) bonus change: removed a bunch of trampoline assembly

Comment on attachment 584896 [details] [diff] [review] part 1: fix crashes Review of attachment 584896 [details] [diff] [review]: ----------------------------------------------------------------- Seems OK. ::: js/src/ion/IonFrameIterator.h @@ +100,5 @@ > // return address that returns back to the current frame). > uint8 *returnAddressToFp() const { > + return *returnAddressToFp_; > + } > + uint8 **addressOfReturnToFp() const { These names: oh my. ::: js/src/ion/x64/Trampoline-x64.cpp @@ +563,5 @@ > masm.loadValue(Address(esp, 0), JSReturnOperand); > masm.freeStack(sizeof(Value)); > } > > + // Check if the callign frame has been invalidated, in which case we can't nit: calling

Comment on attachment 584897 [details] [diff] [review] part 2: fix stack bugs Review of attachment 584897 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonFrames.cpp @@ +121,5 @@ > > +int32 > +FrameRecovery::OffsetOfSlot(int32 slot) > +{ > + if (slot <= 0) Could we have a comment here explaining that <= 0 means "is an argument"? @@ +122,5 @@ > +int32 > +FrameRecovery::OffsetOfSlot(int32 slot) > +{ > + if (slot <= 0) > + return sizeof(IonJSFrameLayout) + -slot; Should this be "+ (-slot * STACK_SLOT_SIZE)", in order to be an offset? ::: js/src/ion/shared/CodeGenerator-shared.cpp @@ +89,5 @@ > + if (a->isStackSlot()) { > + JS_ASSERT(a->toStackSlot()->slot() >= 1); > + return a->toStackSlot()->slot(); > + } > + return -a->toArgument()->index(); Prefer -(a->toArgument()->index()) to make the negative harder to overlook. ::: js/src/ion/x64/Trampoline-x64.cpp @@ -431,5 @@ > > - // Convert the remaining header to an exit frame. Stack is: > - // ... > - // +8 descriptor > - // +0 returnAddress Nice =]

(In reply to Sean Stangl from comment #6) > Comment on attachment 584897 [details] [diff] [review] > part 2: fix stack bugs > > Review of attachment 584897 [details] [diff] [review] > Should this be "+ (-slot * STACK_SLOT_SIZE)", in order to be an offset? Arguments always have offsets instead of indexes. I'll comment that so it's clear.

Comment on attachment 584896 [details] [diff] [review] part 1: fix crashes Review of attachment 584896 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/Trampoline-arm.cpp @@ +529,5 @@ > uint32 argumentPadding = (f.explicitArgs % (StackAlignment / sizeof(void *))) * sizeof(void *); > if (f.explicitArgs) { > argsBase = regs.takeAny(); > + masm.ma_add(sp, Imm32(sizeof(IonExitFrameLayout) + argumentPadding + sizeof(uintptr_t)), > + argsBase); Pushing an extra value will cause the stack to be misaligned. It shuold also be totally legit to stuff this into the padding of the IonCommonFrame. It'll always be at the same location relative to the current sp, and it won't throw stuff out of alignment. also, ma_push does not update framePushed, so this will throw setupAlignedABICall out of whack. Shouldn't you be including the return address in the calculation of argumentPadding? Correction: we aren't actually using argumentPadding here, we're just calculating the value that was used elsewhere so we know how far up the stack to look. Since this confused me, perhaps a comment is in order? @@ +578,5 @@ > + Label invalidated; > + masm.ma_pop(r5); > + masm.ma_cmp(r5, Operand(sp, 0)); > + masm.ma_b(Assembler::NotEqual, &invalidated); > + This is fine, but since {r4..r11,r13} are preserved across function calls, technically speaking you can just leave the value in one of those registers and it will still be there after the call. @@ +586,5 @@ > masm.handleException(); > > + masm.bind(&invalidated); > + masm.ret(); > + The other path uses retn, which will pop the pc, and remove this frame and its arguments. I suspect that you want to do the same thing here.

> The other path uses retn, which will pop the pc, and remove this frame and > its arguments. I suspect that you want to do the same thing here. We can't use retn here, because we have to keep the frame descriptor intact.

http://hg.mozilla.org/projects/ionmonkey/rev/3d3b77875d9c http://hg.mozilla.org/projects/ionmonkey/rev/57eab7d592f7 (This appears to fix about 5 test failures on tbpl.)

OOC, did we happen to create a minimal test case for this?