Last Comment Bug 713997 - IonMonkey: On-Stack Invalidation crash in jaeger/inline/undefinedLocal.js
: IonMonkey: On-Stack Invalidation crash in jaeger/inline/undefinedLocal.js
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 677337 IonOSI
  Show dependency treegraph
 
Reported: 2011-12-28 18:57 PST by David Anderson [:dvander]
Modified: 2012-01-04 11:05 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: fix crashes (10.57 KB, patch)
2011-12-29 18:18 PST, David Anderson [:dvander]
sstangl: review+
marty.rosenberg: review-
Details | Diff | Splinter Review
part 2: fix stack bugs (21.07 KB, patch)
2011-12-29 18:22 PST, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review
part 1: fix stack bugs (ARM take 2) (10.81 KB, patch)
2011-12-30 16:48 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 1: fix stack bugs (ARM take 2), actual patch (10.46 KB, patch)
2011-12-30 16:52 PST, David Anderson [:dvander]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-12-28 18:57:07 PST
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.
Comment 1 David Anderson [:dvander] 2011-12-28 19:13:48 PST
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.
Comment 2 David Anderson [:dvander] 2011-12-29 18:18:41 PST
Created attachment 584896 [details] [diff] [review]
part 1: fix crashes

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 3 David Anderson [:dvander] 2011-12-29 18:19:14 PST
Comment on attachment 584896 [details] [diff] [review]
part 1: fix crashes

Marty, could you check the ARM changes?
Comment 4 David Anderson [:dvander] 2011-12-29 18:22:47 PST
Created attachment 584897 [details] [diff] [review]
part 2: fix stack bugs

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 5 Sean Stangl [:sstangl] 2011-12-30 14:46:54 PST
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 6 Sean Stangl [:sstangl] 2011-12-30 15:05:41 PST
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 =]
Comment 7 David Anderson [:dvander] 2011-12-30 16:29:53 PST
(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 8 Marty Rosenberg [:mjrosenb] 2011-12-30 16:39:01 PST
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.
Comment 9 David Anderson [:dvander] 2011-12-30 16:47:35 PST
> 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.
Comment 10 David Anderson [:dvander] 2011-12-30 16:48:15 PST
Created attachment 585079 [details] [diff] [review]
part 1: fix stack bugs (ARM take 2)
Comment 11 David Anderson [:dvander] 2011-12-30 16:52:41 PST
Created attachment 585080 [details] [diff] [review]
part 1: fix stack bugs (ARM take 2), actual patch
Comment 12 David Anderson [:dvander] 2011-12-30 17:54:48 PST
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.)
Comment 13 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 11:05:33 PST
OOC, did we happen to create a minimal test case for this?

Note You need to log in before you can comment on or make changes to this bug.