Bug 686927 (IonOSI)

IonMonkey: On-Stack Invalidation

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dvander, Assigned: cdleary)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
IonMonkey frames exist on the C stack, and when going into the VM, we may need to recover those frames. Unfortunately, moving those frames to the interpreter stack would break the contiguous stack ordering.

Plan of attack: while StackIter is iterating, it should detect if the next frame it *should* be visiting is actually an Ion frame. At that point, we should convert all Ion frames in its IonActivation to heap-allocated js::StackFrames, similar to generators. Then we make StackIter able to iterate those.

From there, ThunkToInterpreter should just return out of *all* Ion frames, back to ion::Cannon. From there, ion::Cannon should push all of those heap frames onto the interpreter stack, and if there's a pending error, process it.

A few bits of trickiness:
 (1) The first Ion frame already has an interpreter frame, so be careful not to push it twice. Converting it from heap->stack in-place would be safest.
 (2) I think ion::Cannon is responsible for handling errors. If it reaches the original frame without a handler, it just returns false - otherwise, it has to re-enter the interpreter. Returning back to the interpreter would be tricky in the RunScript case.
(Reporter)

Comment 1

6 years ago
More trickiness:
 (3) The topmost frame, from a bailout, is really a fake Ion frame. Don't bailout that.
 (4) C++ calls will be able to re-use this mechanism, but we'll need to take care to 
     handle the return value from the C++ call. We can worry about that in a separate bug.
Created attachment 560426 [details]
Test case: Bail with multiple ion frames.
(Reporter)

Comment 3

6 years ago
s/FindExceptionHandler/js_InternalThrow
(Reporter)

Updated

6 years ago
Blocks: 683037

Updated

6 years ago
Duplicate of this bug: 691598
(Reporter)

Updated

6 years ago
Attachment #560426 - Attachment is patch: false
(Reporter)

Updated

6 years ago
Depends on: 695075
This bug is morphing a bit -- stack iteration is being separated into its own bug, and this will be purely about the on-stack invalidation mechanism that, in a sense, "target aborts" a frame while it's active on the stack, without having to bail out every frame that's newer than it.

The current plan is to swap the return address for the *callee* of an invalidated frame (*caller*) with the address of a thunk that bails out the caller. The original return address has to be stashed away for use by the thunk, for which we'll use a heap-allocated tuple of (calleeToken, returnAddress) and replace the calleeToken value with a tagged version of this tuple pointer.

The thunk bails out the invalidate frame, then thunks to the interpreter, and returns to the invalidated frame's caller.
Assignee: general → cdleary
No longer depends on: 695075
Note that this invalidation procedure is fallible due to malloc.

When invalidation fails, we flag the offending IonActivation as "failed invalidation", which will be checked in the epilogue of the exit frame (which is on the return path from C++ back to Ion code).

Failing due to invalidation will cause us to perform the same procedure as getting a |false| return value from the VM function -- pop all the frames in the IonActivation and return false to the ion::Cannon caller.
(Reporter)

Updated

6 years ago
Summary: IonMonkey: On-Stack Invalidation for Bailouts → IonMonkey: On-Stack Invalidation
(Reporter)

Updated

6 years ago
Attachment #560426 - Attachment is obsolete: true
Created attachment 578155 [details] [diff] [review]
WIP: OSI.

Day one progress. Scaffolding for changing the callee token to the malloc'd invalidation record. Haven't finished with the JS frame layout additions or implemented the thunk yet, so very much WIP. :-)
Depends on: 706986
Note: need to fix the failure status of this test once OSI is in place:

https://hg.mozilla.org/projects/ionmonkey/rev/188a24073bf9
(Reporter)

Updated

6 years ago
Blocks: 708455
Alias: IonOSI
Working note: the postSnapshot (attached to the MIR via a resumeAfter call) encodes the interpreter state *after* the side effecting instruction has executed. So, in the example of a JSOP_GETPROP that successfully calls a scripted getter that happens to invalidate its caller, the snapshot will encode the return registers from the VM call with the corresponding interpreter slot pushed by that bytecode.

However, the regs have already been saved across the call by one of two mechanisms:

- The "inline" call mechanism, which uses the register allocator to put values back in their canonical, register-allocation backing stack locations.
- The "out of line" call mechanism, which explicitly saves the register state away before calling out to the VM.

So, there's the issue of merging the return values from the callee (placed in registers) with the machine context that has been saved away by one of the mechanisms above.

In the inline call case, the register allocator has stashed everything into its backing store stack slots, with the sole exception of the return value from the callee, which remains in registers. As a result, we can use the register state that's preserved by the invalidation thunk directly, because those are, by definition, the only registers that will be accessed by that snapshot.

In the out-of-line case it's a little bit trickier, because we have to merge the pre-call registers with the post-call return registers to get an appropriate register context. Since there can be a maxmium of two return registers, we can just add a few bits to the postSnapshot to indicate how to appropriately merge the out-of-line register sets together.

The postSnapshot can be found for a given call by taking the prior return address (stashed away in the invalidation record) and using the frameInfo table to determine (by distance from the ionScript base code address) the corresponding post-snapshot.
Created attachment 582422 [details] [diff] [review]
WIP: OSI.

Yay, working OSI for inline calls. My recursive invalidation test case passes with this patch. Still needs multi-arch support and some cleanup.
Attachment #578155 - Attachment is obsolete: true
Before I can land this I also need to modify HandleException to traverse the Ion stack and free the InvalidationRecords that have been allocated as well as the IonScripts that are associated with them.
Created attachment 582986 [details] [diff] [review]
WIP: OSI.

x64 and x86 support. Now extending HandleException to walk Ion frames in the activation to delete invalidation records.
Attachment #582422 - Attachment is obsolete: true
Created attachment 582994 [details] [diff] [review]
On Stack Invalidation for x86 and x64.
Attachment #582986 - Attachment is obsolete: true
Attachment #582994 - Flags: review?(dvander)
(Reporter)

Comment 14

6 years ago
Comment on attachment 582994 [details] [diff] [review]
On Stack Invalidation for x86 and x64.

Review of attachment 582994 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work. This is fairly hairy stuff but the patch is quite readable. Some random suggestions:

::: js/src/ion/Bailouts.cpp
@@ +306,5 @@
> +    IonCompartment *ioncompartment = cx->compartment->ionCompartment();
> +    IonActivation *activation = cx->threadData()->ionActivation;
> +    FrameRecovery in = FrameRecoveryFromInvalidation(ioncompartment, sp);
> +
> +    JS_ASSERT(calleeRetval == !!calleeRetval); // Callee's retval must be boolean.

This assert might not hold, we return Values in ecx:edx or rcx and eax/rax could be garbage.

@@ +318,5 @@
> +    uint32 retval = ConvertFrames(cx, activation, in);
> +
> +    // We've bailed out the invalidated frame, so we now transform it into an exit frame.
> +    // Since the callee token isn't part of the exit frame structure, we have to bump the caller
> +    // frame size to account for the "extra" caller token word.

Good observation. Making a mental note here that we should make ::Bailout look the same and move its similar fixup to C++.

::: js/src/ion/Ion.cpp
@@ +862,5 @@
>  
> +static void
> +FailInvalidation(JSContext *cx, uint8 *ionTop)
> +{
> +    JS_ASSERT(false); // NYI

Follow-up bug needed, we can land without this though

@@ +914,5 @@
> +        IonSpew(IonSpew_Invalidate, "   ! requires invalidation");
> +        JS_ASSERT(CalleeTokenGetTag(it.calleeToken()) != CalleeToken_InvalidationRecord);
> +        JS_ASSERT(CalleeTokenMatchesScript(it.calleeToken(), script));
> +        InvalidationRecord *record =
> +            OffTheBooks::new_<InvalidationRecord>(it.calleeToken(), *calleeReturnAddressPtr);

You'll need to update 
        $(srcdir)/config/check_source_count.py OffTheBooks:: 60 \

in Makefile.in

@@ +916,5 @@
> +        JS_ASSERT(CalleeTokenMatchesScript(it.calleeToken(), script));
> +        InvalidationRecord *record =
> +            OffTheBooks::new_<InvalidationRecord>(it.calleeToken(), *calleeReturnAddressPtr);
> +        if (!record) {
> +            FailInvalidation(cx, ionTop);

Instead, could this function return false, and the caller be responsible for running FailInvalidation?

::: js/src/ion/IonFrames.cpp
@@ +73,5 @@
> +      case CalleeToken_Script:
> +        ionScript = CalleeTokenToScript(calleeToken)->ion;
> +        break;
> +      case CalleeToken_Function:
> +        ionScript = CalleeTokenToFunction(calleeToken)->script()->ion;

This logic could be simplified by being hoisted into a function like MaybeScriptFromCalleeToken. That could also replace CalleeTokenMatchesScript.

@@ +199,5 @@
> +        return script;
> +      }
> +      default:
> +        JS_NOT_REACHED("invalid tag");
> +        return NULL;

Another place MaybeScriptFromCalleeToken could help.

@@ +263,5 @@
>      return *this;
>  }
>  
>  void
> +IonFrameIterator::setInvalidationRecord(InvalidationRecord *record)

This function belongs in IonJSFrameLayout, I think.

@@ +288,5 @@
>  
>      IonFrameIterator iter(JS_THREAD_DATA(cx)->ionTop);
> +    while (iter.type() != IonFrame_Entry) {
> +        if (iter.type() == IonFrame_JS) {
> +            IonJSFrameLayout *fp = (IonJSFrameLayout *) iter.fp();

This cast is low-level enough such that you might want to make an accessor on IonFrameIterator.

::: js/src/ion/IonFrames.h
@@ +62,5 @@
> +    CalleeToken_InvalidationRecord = 0x2
> +};
> +
> +static inline CalleeTokenTag
> +CalleeTokenGetTag(CalleeToken token)

I think I prefer the old API more, since it exposes less details about callee tokens. I.e. IsCalleeTokenFunction() is more readable than extracting the tag and then comparing the tag. Note the js::Value API is similar, we rarely ask for tags.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +230,5 @@
>  IonCode *
> +IonCompartment::generateInvalidator(JSContext *cx)
> +{
> +    MacroAssembler masm(cx);
> +    JS_ASSERT(false); // NYI

Should file a follow-up bug (I'd at least make sure ARM compiles, though).

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +838,5 @@
>          static const VMFunction InvokeFunctionInfo = FunctionInfo<pf>(InvokeFunction);
>  
>          // Nestle %esp up to the argument vector.
> +        if (unusedStack)
> +            masm.addPtr(Imm32(unusedStack), StackPointer);

It looks like you'll probably land before me - this should be reserveStack, and freeStack, everywhere StackPointer is modified in this function. Otherwise IonFrameIterator will be busted. Might as well fix it here.

::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +84,5 @@
>          return calleeToken_;
>      }
> +    void setCalleeToken(void *value) {
> +        calleeToken_ = value;
> +    }

I'd almost argue using "update" or "replace" instead of "set" here, since we're basically hijacking the frame rather than casually setting some random stuff. Similarly, it might be nicer to have a setter for the return address instead of exposing its address (unless that's necessary).

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +212,5 @@
> +    // The return value from Bailout is tagged as:
> +    // - 0x0: done (thunk to interpreter)
> +    // - 0x1: error (handle exception)
> +    // - 0x2: reflow args
> +    // - 0x3: reflow barrier

What happens if we invalidate the current frame, while inside a normal bailout? It seems like what might happen is that we'll place an invalidation record (since the frame still exists, technically) and then the bailout tail will remove it, leaking the record. If that's true an easy fix would be a special case near the end of ion::Bailout.

@@ +280,5 @@
> +    //        +--------------------------+
> +    //        | invalid frame descriptor |
> +    // esp -> >--------------------------<
> +    //        |       old retaddr        |
> +    //        \--------------------------/

This diagram is awesome, no lie

::: js/src/jsinfer.cpp
@@ +2207,5 @@
>  void
>  TypeCompartment::addPendingRecompile(JSContext *cx, JSScript *script)
>  {
> +#if defined(JS_ION) && !defined(JS_METHODJIT)
> +    if (!script->ion)

This test should take into account ION_DISABLED_SCRIPT (maybe a helper on JSScript would do it)
Attachment #582994 - Flags: review?(dvander) → review+
Created attachment 583571 [details] [diff] [review]
Fail invalidation.

I had forgotten to qref this.
Attachment #583571 - Flags: review?(dvander)
(Reporter)

Comment 16

6 years ago
Comment on attachment 583571 [details] [diff] [review]
Fail invalidation.

Review of attachment 583571 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/Ion.cpp
@@ +815,5 @@
>          else
>              target.enterJIT(jitcode, argc, argv, &result, calleeToken);
>  
>          JS_ASSERT_IF(result.isMagic(), result.isMagic(JS_ION_ERROR));
> +        failedInvalidation = activation.failedInvalidation();

I think this has to be somewhere else, since the failure has to be propagated up immediately. For example, GetProperty could invalidate, but invalidation could fail, and we can't return to the caller.

Basically whenever we create an exit frame we need to check that the activation hasn't been invalidated. So that's generateVMWrapper and in bailouts.
Attachment #583571 - Flags: review?(dvander)
(Reporter)

Comment 17

6 years ago
Err, that is, whenever we are about to return from an exit frame.
Blocks: 712845
Blocks: 712846
https://hg.mozilla.org/projects/ionmonkey/rev/2e74563aa784
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
ARM fixups: https://hg.mozilla.org/projects/ionmonkey/rev/f99cd892afdc
(Reporter)

Updated

6 years ago
Depends on: 713068

Updated

6 years ago
Depends on: 713870
(Reporter)

Updated

6 years ago
Depends on: 713997

Updated

6 years ago
Depends on: 715111
Blocks: 715357
You need to log in before you can comment on or make changes to this bug.