836781 - BaselineCompiler: Fix bailouts from Ion frames with inlined NEW/FUNAPPLY ops (FUNCALL, once Ion inlines it)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Ion can have inlined frames invoked via a logical JSOP_NEW, JSOP_FUNAPPLY, etc.

When bailing out frames containing these inlined frames, resuming into baseline is buggy because the bailout code assumes we'll always be resuming into the Call_Scripted optimized stub, and we're not guaranteed that we have existing stubs on the stub chain handling the invoke ops being reconstructed.

In fact, even with JSOP_CALL this is a bug because the CALL stub could be gc-collected while ion code is running.

Comment 1

[Kannan Vijayan [:djvj]]

Solution plan:

We just need a unified, guaranteed-to-exist (i.e. not be GCed) way to resume into baseline code immediately AFTER a JOF_INVOKE operation.

We can use the jitcode for the ICCall_Fallback stub to do this.  Currently the jitcode just does a VM tailcall to DoCallFallback(), which handles the call manually and tries to attach stubs.

We can attach an "after-call" resume path after this VM tailcall.  The code will never be reached during actual baseline execution.  However, when bailing out from Ion, it can be used as a guaranteed return-addr that can be rejoined at with the correct semantics for all INVOKE operations.

Fallback stubs are never collected, while the baseline script is around, so we do not have to worry about collection getting in the way either.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: Patch

The fix.

This also fixes a bug in the way arguments in BaselineStub and Rectifier frames were reconstructed when bailing from Ion.  The bug did not exhibit because the badly reconstructed arguments were never used (just discarded from the stack as we returned up the frame stack).

Comment 3

[Kannan Vijayan [:djvj]]

Note: the patch seems to cause a new test failure in x64 jit-tets/check-bitops-nsieve-bits.js  (not a crash, but computing a wrong result).

Couldn't spot anything immediately obvious.  Need to look into that.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 708757 [details] [diff] [review]
Patch

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

Nice.

(In reply to Kannan Vijayan [:djvj] from comment #3)
> Note: the patch seems to cause a new test failure in x64
> jit-tets/check-bitops-nsieve-bits.js  (not a crash, but computing a wrong
> result).

I noticed the same problem on x86 yesterday, so it seems unrelated to this patch.

::: js/src/ion/BaselineBailouts.cpp
@@ +721,4 @@
>      unsigned actualArgc = GET_ARGC(pc);
>      JS_ASSERT(actualArgc + 2 <= exprStackSlots);
>      for (unsigned i = 0; i < actualArgc + 1; i++) {
> +        // size_t argSlot = (script->nfixed + exprStackSlots) - (actualArgc + 1) + i;

Nit: delete comment.

Comment 5

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/projects/ionmonkey/rev/87a78b950c9d

Comment 6

[Paul Wright]

Should this be marked FIXED?