713997 - IonMonkey: On-Stack Invalidation crash in jaeger/inline/undefinedLocal.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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] - inactive, e-mail if emergency]

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] - inactive, e-mail if emergency]

Attachment: 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] - inactive, e-mail if emergency]

Comment on attachment 584896 [details] [diff] [review]
part 1: fix crashes

Marty, could you check the ARM changes?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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]]

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]]

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] - inactive, e-mail if emergency]

(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]]

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] - inactive, e-mail if emergency]

> 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] - inactive, e-mail if emergency]

Attachment: part 1: fix stack bugs (ARM take 2)

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: fix stack bugs (ARM take 2), actual patch

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

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)]

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