The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
Attachment #584896 - Flags: review?(sstangl)
(Assignee)

Comment 3

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

Marty, could you check the ARM changes?
Attachment #584896 - Flags: review?(mrosenberg)
(Assignee)

Comment 4

5 years ago
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
Attachment #584897 - Flags: review?(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
Attachment #584896 - Flags: review?(sstangl) → review+
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 =]
Attachment #584897 - Flags: review?(sstangl) → review+
(Assignee)

Comment 7

5 years ago
(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.
Attachment #584896 - Flags: review?(mrosenberg) → review-
(Assignee)

Comment 9

5 years ago
> 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.
(Assignee)

Comment 10

5 years ago
Created attachment 585079 [details] [diff] [review]
part 1: fix stack bugs (ARM take 2)
Attachment #584896 - Attachment is obsolete: true
Attachment #585079 - Flags: review?(mrosenberg)
(Assignee)

Comment 11

5 years ago
Created attachment 585080 [details] [diff] [review]
part 1: fix stack bugs (ARM take 2), actual patch
Attachment #585079 - Attachment is obsolete: true
Attachment #585079 - Flags: review?(mrosenberg)
Attachment #585080 - Flags: review?(mrosenberg)
Attachment #585080 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 12

5 years ago
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.)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
OOC, did we happen to create a minimal test case for this?
You need to log in before you can comment on or make changes to this bug.