IonMonkey: unexpected argument types should be barriered / cause reflow




JavaScript Engine
6 years ago
6 years ago


(Reporter: cdleary, Assigned: cdleary)


Firefox Tracking Flags

(Not tracked)



(2 attachments)

This came up when designing a test for OSI:

var causeOSI = true;

function rec(x, self) {
    if (x === 0) {
        if (causeOSI) {
            causeOSI = false;
            self(NaN, self)
            causeOSI = true;
    self(x - 1, self);

// Use enough iterations to type infer the script.
causeOSI = false;
for (var i = 0; i < 20; ++i)
    rec(1, rec);
causeOSI = true;

rec(2, rec)

We get an infer failure because the argument type mismatch causes a bailout (that thunks to the interpreter) without reflowing the type information first.

We can create a pretty simple, common thunk that reflows all the type information for all the formal arguments when any of them mismatch. When the types are *not* a mismatch type inference should just perform a single check per argument, so it doesn't seem worth the overhead to pass information on which arguments were mismatched.
Just chatted with Brian: he says that freezing the argument types should be okay in terms of triggering recompilation (instead of an IC-repatching based approach that was used in JM+TI, to prevent recompilation for compatible-but-expanding typesets).

With that in mind, we can embed type checks into the prologue and call out to a ReflowArgumentTypes thunk, passing the current script as an argument. If the script is invalidated by the type reflow then the thunk will return an error code, causing the ion code to take a Bailout.

In JM+TI this type-checking code is a separate entry point, so that callers which know definitively that their actual arguments are compatible with the typesets of the callee's formal arguments can bypass these checks.
Ion scripts have the separate entry-point as well, but the type checks are inlined. If they fail we take a bailout. The problem is there is no bailout mechanism to inform of argument type-check failure. Worse, invalidation inside a bailout is unsafe, because there is no exit frame.

The call to ReflowArgumentTypes, instead of performing a bailout, would solve both these problems, as long as invalidation was guaranteed (if not we'd need a bailout after). But this is also an existing problem for type barriers. An easy fix would be to create an exit frame for ion::Bailout, but that might mess up the return path when invalidation occurs. Another idea is to return some TI data from ion::Bailout, and in the bailout assembly, create an exit frame and call to a TI handler.
Talked with David and I'm going to try the VM-call-to-TI-reflow-after-exit-frame-has-been-created route. A different bailout status code will be returned in the case where reflow is required.

The Bailout call will be extended to return some algebraic thing like TypeBarrier(pcOffset) | ReflowArgs | Error. Handwaving a bit on that part for now, since I assume there's a way to make it happen. :-)
Created attachment 578771 [details] [diff] [review]
Reflow after bailout.

This appears to work. Of course, without invalidation we currently fail this due to "too much recursion" because of thunking to the interpreter that re-enters the ion code that thunks to the interpreter...
Attachment #578771 - Flags: review?(dvander)
Comment on attachment 578771 [details] [diff] [review]
Reflow after bailout.

Review of attachment 578771 [details] [diff] [review]:

Nice - a few nits, but one non-nit: Trampoline-x64 and Trampoline-arm need to be modified too.

::: js/src/ion/Bailouts.h
@@ +134,5 @@
>      Bailout_Normal,
> +    // A bailout at the very start of a function indicates that there may be
> +    // a type mismatch in the arguments that necessitates a reflow.
> +    Bailout_Entry,

Bailout_ArgumentCheck or something would be clearer.

@@ +152,4 @@
>  static const uint32 BAILOUT_RETURN_OK = 0;
>  static const uint32 BAILOUT_RETURN_FATAL_ERROR = 1;
> +static const uint32 BAILOUT_RETURN_ENTRY = 2;
> +static const uint32 BAILOUT_RETURN_TYPE_BARRIER = 3;


::: js/src/ion/x86/Trampoline-x86.cpp
@@ +368,5 @@
> +    // - 0x1: error (handle exception)
> +    // - 0x2: reflow args
> +    // - 0x3: reflow barrier
> +
> +    masm.cmpl(eax, Imm32(0x1));

Attachment #578771 - Flags: review?(dvander) → review+
Blocks: 686927
Created attachment 579161 [details] [diff] [review]
ARM reflow.

The patch for x64 is the same, s/eax/rax, but the ARM one is sufficiently different to warrant a review. :-)
Attachment #579161 - Flags: review?
Attachment #579161 - Flags: review? → review?(mrosenberg)
Comment on attachment 579161 [details] [diff] [review]
ARM reflow.

Review of attachment 579161 [details] [diff] [review]:

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +410,5 @@
> +    // - 0x2: reflow args
> +    // - 0x3: reflow barrier
> +
> +    masm.ma_cmp(r0, Imm32(BAILOUT_RETURN_FATAL_ERROR));
> +    masm.as_b(&interpret, Assembler::LessThan);

Could you use ma_b rather than as_b?
ma_* is from the macro assembler, whereas as_* is from the assembler, and in general, harder to use correctly.  My plan is to mark everything in the assembler as protected so *only* the macro assembler can use it.
Attachment #579161 - Flags: review?(mrosenberg) → review+
Last Resolved: 6 years ago
Resolution: --- → FIXED
The test is known failing until we have OSI:
You need to log in before you can comment on or make changes to this bug.