843483 - BaselineCompiler: Correctly forward BailoutKind when bailing out from Ion into Baseline.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

When an Ion script bails out in trunk, the bailoutTail asmcode is passed a return value that reflects the bailout kind.  The asmcode in turn uses that value to invoke an appropriate post-bailout handler function.

Many of these BailoutKinds and associated handlers (recompile check, bounds check failure, shape guard failure, etc.) invalidate the script being bailed out from, which allows for recompilation.

When these bailouts occur to baseline, the kind is ignored, the script marked with 'bailoutExpected', and execution resumed.  Thus bailed out ion scripts never get recompiled.

When bailing out to baseline, code should preserve the BailoutKind and trigger actions similar to those triggered when bailing out to interpreter.

Comment 1

[Jan de Mooij [:jandem]]

Nice find.

Maybe we can store the bailoutKind in BaselineBailoutInfo and have FinishBailoutToBaseline switch on that..

Comment 2

[Kannan Vijayan [:djvj]]

Yeah, that's what I was thinking too.

We'll need to the keep the top script that was bailed out around too, since we can't rely on cx->ionTop to obtain it.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Handle BailoutKind when bailing from ion to baseline.

Turns out that the script doesn't need to be kept around after all, since BaselineBailoutInfo contains the number of frames bailed out, and the stack is walked anyway to close iterators.  Top and bottom script can be obtained from the from the callee token during the stack walk.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 716934 [details] [diff] [review]
Handle BailoutKind when bailing from ion to baseline.

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

Nice, I'm glad we can fix this without touching the BailoutTail asm code. r=me with comments addressed.

::: js/src/ion/BaselineBailouts.cpp
@@ +1021,5 @@
> +{
> +    IonSpew(IonSpew_Bailouts, "Bounds check failure %s:%d", script->filename, script->lineno);
> +
> +    if (!script->failedBoundsCheck) {
> +        script->failedBoundsCheck = true;

GetBailedJSScript returns the outermost script, so should this be bottomScript? Same for the two functions below. I think some compilers will complain about the unused argument so I'd just remove it.

Also, it would be nice if we could move these functions to Bailouts.cpp and have the functions there call these ones.

@@ +1113,5 @@
> +            if (frameno == 0)
> +                bottomScript = frame->script();
> +
> +            if (frameno == numFrames - 1)
> +                topScript = frame->script();

Hm, I think bottomScript should be topScript and topScript should be bottomScript here? Iteration starts at the "most recent" frame and we call this the "top frame" elsewhere (even though it's at the bottom of the physical C stack). When reading the rest of the code, I thought topScript was the "most recent", inlined frame.

Else rename them to outerScript/innerScript, seems a bit less ambiguous.

@@ +1147,5 @@
> +        // at the top-level, because usesBeforeInlining() < usesBeforeCompile.
> +        JS_NOT_REACHED("Unexpected recompile check!");
> +        break;
> +      case Bailout_BoundsCheck:
> +        HandleBoundsCheckFailure(cx, topScript, bottomScript);

Propagate failure from this call and the two calls below.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Try 2

Resubmitting this for review because after addressing comments above, I made some other behavioural changes which differ from Ion's behaviour.

Ion always sets the flag on, and invalidates, the inner script (i.e. the deepest inlined script which triggered the invalidation).

This doesn't seem right.  What actually received the bailout was the jitcode for the outer-most script - and it seems appropriate that it should be invalidated too.  Also, the JS_ASSERT in Ion that the inner script has ionCode seems bogus.  It's unlikely but possible that the inner script as never been ioncompiled, and only has jitcode inlined into its callees.

The new patch just changes the behaviour on how we handle the BailoutKind.  It sets the flag on the innermost script, asserts that the outermost script has jitcode, and invalidates that jitcode.. and if the innermost script has jitcode it invalidates that too (but it doesn't assert that the innermost script has jitcode).

I'm trying to induce a crash in trunk Ion based on that expectation, but I can't quite construct a case that does it.

Can someone with more exposure to this stuff look at the handling of this in trunk Ion and comment on the above thoughts?

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: Try 3, after minor fix

Got a couple of lines backwards in the previous patch.  Just fixing it and posting again.

Read comment above.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> Ion always sets the flag on, and invalidates, the inner script (i.e. the
> deepest inlined script which triggered the invalidation).

As far as I can see, GetBailedJSScript will return the outer script (it looks at the frame's callee token).

Comment 8

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Kannan Vijayan [:djvj] from comment #5)
> > Ion always sets the flag on, and invalidates, the inner script (i.e. the
> > deepest inlined script which triggered the invalidation).
> 
> As far as I can see, GetBailedJSScript will return the outer script (it
> looks at the frame's callee token).

Yeah, I see that now.  For some reason I was imagining the ionTop pointer as referring to the innermost frame of the ioncode that generated the bailout.

Will take dvander off the needinfo.  What do you think about the rest of the changes?  It seems to be the right behaviour to invalidate both the inner script's and outer script's jitcode, and set the flag on the inner script.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 717288 [details] [diff] [review]
Try 3, after minor fix

(In reply to Kannan Vijayan [:djvj] from comment #8)
> What do you think about the rest of the
> changes?  It seems to be the right behaviour to invalidate both the inner
> script's and outer script's jitcode, and set the flag on the inner script.

I think we should follow m-c for now, or land these changes on m-c first. These things are a bit delicate and I'm afraid they could introduce subtle regressions on some benchmarks..

Comment 10

[Kannan Vijayan [:djvj]]

Gotcha.  Previously r+-ed patch fixed up according to comments and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/1f1f815eb9e8

Comment 11

[Paul Wright]

Is this fixed or is there more to do here?

Comment 12

[Kannan Vijayan [:djvj]]

Fixed.  Was waiting for tbpl green but forgot about it.  Thanks for the reminder.  Closing now.