Last Comment Bug 686927 - (IonOSI) IonMonkey: On-Stack Invalidation
: IonMonkey: On-Stack Invalidation
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
: Jason Orendorff [:jorendorff]
Depends on: 706986 713068 713870 713997 715111
Blocks: IonMonkey IM+TI 708455 712845 712846 715357
  Show dependency treegraph
Reported: 2011-09-15 12:00 PDT by David Anderson [:dvander]
Modified: 2012-01-04 14:59 PST (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test case: Bail with multiple ion frames. (392 bytes, text/plain)
2011-09-15 12:06 PDT, Sean Stangl [:sstangl]
no flags Details
WIP: OSI. (14.31 KB, patch)
2011-11-30 18:20 PST, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Splinter Review
WIP: OSI. (47.01 KB, patch)
2011-12-16 15:53 PST, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Splinter Review
WIP: OSI. (55.92 KB, patch)
2011-12-19 15:40 PST, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Splinter Review
On Stack Invalidation for x86 and x64. (56.63 KB, patch)
2011-12-19 16:24 PST, Chris Leary [:cdleary] (not checking bugmail)
dvander: review+
Details | Diff | Splinter Review
Fail invalidation. (2.15 KB, patch)
2011-12-21 11:44 PST, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Splinter Review

Description User image David Anderson [:dvander] 2011-09-15 12:00:16 PDT
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.
Comment 1 User image David Anderson [:dvander] 2011-09-15 12:06:20 PDT
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.
Comment 2 User image Sean Stangl [:sstangl] 2011-09-15 12:06:24 PDT
Created attachment 560426 [details]
Test case: Bail with multiple ion frames.
Comment 3 User image David Anderson [:dvander] 2011-09-15 12:08:12 PDT
Comment 4 User image Sean Stangl [:sstangl] 2011-10-03 18:12:17 PDT
*** Bug 691598 has been marked as a duplicate of this bug. ***
Comment 5 User image Chris Leary [:cdleary] (not checking bugmail) 2011-11-30 15:10:01 PST
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.
Comment 6 User image Chris Leary [:cdleary] (not checking bugmail) 2011-11-30 15:13:05 PST
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.
Comment 7 User image Chris Leary [:cdleary] (not checking bugmail) 2011-11-30 18:20:29 PST
Created attachment 578155 [details] [diff] [review]

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. :-)
Comment 8 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-07 11:26:13 PST
Note: need to fix the failure status of this test once OSI is in place:
Comment 9 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-09 17:56:59 PST
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.
Comment 10 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-16 15:53:34 PST
Created attachment 582422 [details] [diff] [review]

Yay, working OSI for inline calls. My recursive invalidation test case passes with this patch. Still needs multi-arch support and some cleanup.
Comment 11 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-16 16:31:08 PST
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.
Comment 12 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-19 15:40:06 PST
Created attachment 582986 [details] [diff] [review]

x64 and x86 support. Now extending HandleException to walk Ion frames in the activation to delete invalidation records.
Comment 13 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-19 16:24:24 PST
Created attachment 582994 [details] [diff] [review]
On Stack Invalidation for x86 and x64.
Comment 14 User image David Anderson [:dvander] 2011-12-20 17:14:43 PST
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/ OffTheBooks:: 60 \


@@ +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)
Comment 15 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-21 11:44:38 PST
Created attachment 583571 [details] [diff] [review]
Fail invalidation.

I had forgotten to qref this.
Comment 16 User image David Anderson [:dvander] 2011-12-21 12:22:50 PST
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.
Comment 17 User image David Anderson [:dvander] 2011-12-21 12:23:17 PST
Err, that is, whenever we are about to return from an exit frame.
Comment 18 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-21 19:21:06 PST
Comment 19 User image Chris Leary [:cdleary] (not checking bugmail) 2011-12-21 21:17:29 PST
ARM fixups:

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