621022 - Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'eqi' is 'immd' which has type double (expected int)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

---
function f(x) {
    delete arguments[0];
    arguments[0] !== undefined;
}

for(var i=0; i<20; i++) {
    f(1);
}
---
Asserts with -j:

Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'eqi' is 'immd' which has type double (expected int): 0 (../nanojit/LIR.cpp:3207)

Comment 1

[Jeff Walden [:Waldo]]

Feel free to assign this to me if it gets marked blocking.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

As a precaution, LIR type errors are generally filed security sensitive.

Comment 3

[Nicholas Nethercote [inactive]]

The problem is here:

00010:   3  strictne
    ------------------------------ # JSOP_STRICTNE
    eqi5 = eqi immd1/*1*/, strict/*0*/    ## type error
    eqi6 = eqi eqi5, strict/*0*/
    sti.sp sp[40] = eqi6

The "strict" name is misleading, because of CSE every zero immediate ends up sharing the same name.  Pretend it looks like this:

00010:   3  strictne
    ------------------------------ # JSOP_STRICTNE
    eqi5 = eqi immd1/*1*/, undefined/*0*/    ## type error
    eqi6 = eqi eqi5, immi0/*0*/
    sti.sp sp[40] = eqi6

"undefined" is represent by an integer zero, as expected.  The 1 argument is represented as a double 1.  So presumably the 1 should be demoted or the 0 should be promoted before the comparison occurs.

In TraceRecorder::strictEquality(), ltag and rtag are JSVAL_TYPE_UNDEFINED, so we end up in generating an eqi comparison.

Comment 4

[Nicholas Nethercote [inactive]]

BTW, as well as !==, the type errors also occur with !=, ==, ===.

Comment 5

[Nicholas Nethercote [inactive]]

The fact that the ltag is JSVAL_TYPE_UNDEFINED seems wrong.

Comment 6

[David Mandelin [:dmandelin]]

Is this fairly easy to fix?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> Is this fairly easy to fix?

Not sure yet, I don't understand the root cause.

Comment 8

[Nicholas Nethercote [inactive]]

A bit more context:

......:   3  argsub 0
    ------------------------------ # JSOP_ARGSUB
    sti.sp sp[40] = immi1/*1*/
    
......:   3  getgname "undefined"
    ------------------------------ # JSOP_GETGNAME
    sti.sp sp[48] = strict/*0*/
    
......:   3  strictne
    ------------------------------ # JSOP_STRICTNE
    eqi5 = eqi immd1/*1*/, strict/*0*/
    eqi6 = eqi eqi5, strict/*0*/
    sti.sp sp[40] = eqi6


JSOP_ARGSUB gets the zeroth arg, which is the 1.  It writes it to sp[40] as an immi so it's surprising that JSOP_STRICTNE gets the same value as an immd.

Comment 9

[Nicholas Nethercote [inactive]]

Luke, CCing you as you touched record_JSOP_ARGSUB recently.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 58628:d377404f7ef0)

I think I know what's going on.  First of all, here's the context prior to the ARGSUB (see comment 7):


00000:   2  arguments
    ------------------------------ # JSOP_ARGUMENTS
    globalObj = immi 0xf6d02028
    js_NewArgumentsOnTrace1 = calli.all #js_NewArgumentsOnTrace ( cx globalObj/*
0xf6d02028*/ immi1/*1*/ $global0 )
    eqi3 = eqi js_NewArgumentsOnTrace1, strict/*0*/
    xt1: xt eqi3 -> exit=0x95ca3a4 pc=0x95c5c54 imacpc=(nil) sp+40 rp+4 OOM
    sti.sp sp[40] = js_NewArgumentsOnTrace1
    sti.sp sp[24] = js_NewArgumentsOnTrace1

00001:   2  zero
    ------------------------------ # JSOP_ZERO
    immd2 = immd 0
    sti.sp sp[48] = strict/*0*/

00002:   2  delelem
    ------------------------------ # JSOP_DELELEM
    exit = immi 0x95ca3ec
    sti.cx cx[308] = exit/*0x95ca3ec*/
    xbarrier1: xbarrier  -> exit=0x95ca3ec pc=0x95c5c56 imacpc=(nil) sp+56 rp+4
DEEP_BAIL
    DeleteIntKey1 = calli.all #DeleteIntKey ( cx js_NewArgumentsOnTrace1 strict/
*0*/ strict/*0*/ )
    builtinStatus = ldi.state state[76]
    eqi4 = eqi builtinStatus, strict/*0*/
    sti.cx cx[308] = strict/*0*/
    sti.sp sp[40] = DeleteIntKey1
    xf3: xf eqi4 -> exit=0x95ca434 pc=0x95c5c57 imacpc=(nil) sp+48 rp+4 STATUS

00003:   2  pop
    ------------------------------ # JSOP_POP



record_JSOP_ARGSUB() has this code:

    if (!fp->fun()->isHeavyweight()) {
        uintN slot = GET_ARGNO(cx->regs->pc);
        if (slot >= fp->numActualArgs())
            RETURN_STOP_A("can't trace out-of-range arguments");
        stack(0, get(&cx->fp()->canonicalActualArg(slot)));
        return ARECORD_CONTINUE;
    }

The result of |get(&cx->fp()->canonicalActualArg(slot))| is the LIR_immd that produces the 1.  But that's wrong at this point -- because the DELELEM has deleted that slot, effectively changing it to undefined.  The get() should really be getting "undefined", ie. the LIR_immi with value zero.

So I think record_JSOP_DELELEM needs to update the tracking in cx->fp()->canonicalActualArg() somehow.  But that seems difficult because
it's special behaviour required only for |arguments|, because |arguments| is inside the JSStackFrame and so has its individual slots tracked by the tracer, unlike vanilla objects.

A simpler solution is to abort when DELELEM is called on |arguments|.  The attached patch does this, and the test no longer asserts.

Asking Brendan for review, 'cos he's the original author of record_JSOP_DELELEM().

I also wonder if there are any other ways that a value tracked in cx->fp()->canonicalActualArg() can be rendered invalid like this... I see that we abort tracing if we do a SETELEM on the |arguments| object, so that's good.  Any others like that?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do we need to worry about cases where the object DELELEM happens on is not arguments at record time but is at runtime?

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> Do we need to worry about cases where the object DELELEM happens on is not
> arguments at record time but is at runtime?

I don't think so... |arguments| gets special treatment at record-time (the per-slot tracking) but I don't think it needs special treatment at run-time.  But I'm not certain.  (The same question applies to the global object, I don't know what the answer is for it.)

Comment 13

[Nicholas Nethercote [inactive]]

This is a regression, DELELEM wasn't traced in 1.9.2.

Comment 14

[Brendan Eich [:brendan]]

Attachment: need to guard

Nick, I'm short on time but record time guarantees only object type (not function or one of the primitives), so any recording-time class tests must be accompanied by guards to same effect at runtime, as bz suggested. I'm so short on time that I can't seem to write a test showing the guardNotClass calls' necessity -- can you? I will hand this patch back in lieu of review and r+ anything you derive from it.

/be

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > Do we need to worry about cases where the object DELELEM happens on is not
> > arguments at record time but is at runtime?
> 
> I don't think so... |arguments| gets special treatment at record-time (the
> per-slot tracking) but I don't think it needs special treatment at run-time.

Answere din comment 14, but to be clear: importing stack slots including actual arguments does nothing special for the arguments object, which can alias those slots.
 
> But I'm not certain.  (The same question applies to the global object, I don't
> know what the answer is for it.)

Traces are specialized to one and only one global object, and if we detect a shape change at recording time, we abort; if we detect a shape change at runtime (via some get or set using an alias) then we deep-bail.

/be

Comment 16

[Brendan Eich [:brendan]]

So, we could use the deep-bail code in DeleteIntKey instead of two not-class guards. That might be more efficient -- fewer instructions on trace and all the bad-path handling in the optimized-by-the-C++-compiler helper functions DeleteIntKey and DeleteStrKey.

This points out a hazard to beware: delete arguments[i] where i is "0" will take the DeleteStrKey path, so both of these helpers must deep-bail if arguments is the object they're passed at runtime.

/be

Comment 17

[Nicholas Nethercote [inactive]]

> Nick, I'm short on time but record time guarantees only object type (not
> function or one of the primitives), so any recording-time class tests must be
> accompanied by guards to same effect at runtime, as bz suggested.

That is true, but I'm still unconvinced that it's relevant for this record-time problem.

To put it another way -- what can go wrong if we have a vanilla object at record-time and then |arguments| in one of the subsequent (traced) iterations?
AFAICT, ARGSUB won't be used in that case, and it was the record-time interaction between DELELEM and ARGSUB (specifically, the slot tracking) that caused this problem.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> > Nick, I'm short on time but record time guarantees only object type (not
> > function or one of the primitives), so any recording-time class tests must be
> > accompanied by guards to same effect at runtime, as bz suggested.
> 
> That is true, but I'm still unconvinced that it's relevant for this record-time
> problem.

What record-time problem remains if the recorder is patched to abort when obj is an arguments (either class) object?

> To put it another way -- what can go wrong if we have a vanilla object at
> record-time and then |arguments| in one of the subsequent (traced) iterations?
> AFAICT, ARGSUB won't be used in that case, and it was the record-time
> interaction between DELELEM and ARGSUB (specifically, the slot tracking) that
> caused this problem.

I'm fine with Delete{Int,Str}Key both deep-bailing if obj at runtime is ever any arguments object, even if no ARGSUB usage was involved. No big deal, deleting an arguments element is freakishly rare. Or am I misunderstanding your question?

/be

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to comment #18)
> 
> What record-time problem remains if the recorder is patched to abort when obj
> is an arguments (either class) object?

None, AFAICT.  That's what my original patch did! :)

> I'm fine with Delete{Int,Str}Key both deep-bailing if obj at runtime is ever
> any arguments object, even if no ARGSUB usage was involved. No big deal,
> deleting an arguments element is freakishly rare. Or am I misunderstanding your
> question?

I'm thinking that we don't need to change Delete{Int,Str}Key at all.

In other words, the problem is the record-time interaction of DELELEM and ARGSUB, and if we don't see |arguments| at record-time there won't be an ARGSUB op, and so there'll be no problem.  Whether or not |arguments| could be seen during a subsequent traced execution is irrelevant, AFAICT.

Comment 20

[Nicholas Nethercote [inactive]]

In case that's not clear, I think my original patch is sufficient.

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> In case that's not clear, I think my original patch is sufficient.

Closely reasoned, but what if there's an arguments object at record time but not at compile time (so no ARGSUB selection)? Your patch still aborts just because obj is an arguments object. Here, you could use my "no big deal" argument.

Boris's question about what if there's no arguments object evident at compile or record time. The answer that such a case can't go wrong because of no ARGSUB sounds great. But is there no other way for types to be changed behind the trace's back?

Forget about ARGSUB for a  minute and consider accesses to named formal parameters that have arguments[i] aliases.

/be

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: patch, v3 (against 59784:22fc8e6cdc46)

(In reply to comment #21)
> 
> 
> Forget about ARGSUB for a  minute and consider accesses to named formal
> parameters that have arguments[i] aliases.

I found a test case that required the run-time checks, involving aliasing not via the formal parameters, just via normal aliasing.  I added it as a test.

I also changed your patch to do the run-time test in Delete{Int,Str}Key as you suggested.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 501218 [details] [diff] [review]
patch, v3 (against 59784:22fc8e6cdc46)

Excellent! Gotta watch out for those aliases.

Jim Halpert: [watching the stripper arrive in her car] Have you ever seen a stripper before?
Dwight Schrute: Yes, Jennifer Garner portrayed one on "Alias." It was one of her many aliases.
Jim Halpert: Yeah, me neither.

"The Office" (U.S.), "Ben Franklin" (2007)

/be

Comment 24

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/9df93a2a40e5

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/9df93a2a40e5

Comment 27

[Nicholas Nethercote [inactive]]

This is intereesting:  bug 624547 fixed a very similar problem.  And if I back out the changes from this patch (but keep the tests in) the tests all pass.

In other words, my patch here was a partial fix, but the patch in bug 624547 is a more general fix and subsumes it.

Does it sound ok to back-out the non-test parts of this patch?

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

Brendan's fix was for ARGSUB. If you back yours out, does it pass this test?

function f(x) {
    delete arguments[0];
    var e = arguments;
    e[0] !== undefined;
}

for(var i=0; i<20; i++) {
    f(1);
}

If so, sounds good!

Comment 29

[Nicholas Nethercote [inactive]]

It passes.  Both bugs involved ARGSUB;  this one also involved DELELEM and the fix addressed that, which is why it didn't help with the other bug, but the other bug's patch helps here.

Brendan, you happy with this?

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> Brendan, you happy with this?

You mean backing out this bug's patch? Not really.

LeaveTraceIfArgumentsObject strikes me as a very good idea, belt-and-braces if you will. There are potentially C++ and perhaps even JS arguments[0] aliases we haven't tested yet.

The arguments object seems entirely analogous to the global object in the tracer: importing object property slots onto the JIT stack, needing to abort recording and deep-bail trace execution upon hitting an alias hazard.

Without a proof we don't have other ways of aliasing imported arguments, I would keep this bug's patch. File a followup about producing a proof we don't need this patch if you like.

Anyone think otherwise?

/be

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't think we need it - the bug was purely in ARGSUB, as Nick pointed out. In comment #26 I meant "delete e[0]", but even that will pass. We guard against reading holes already.

Comment 32

[Brendan Eich [:brendan]]

The issue isn't holes, just aliases we don't know about. The arguments object is a bit better confined than the global object, in API and VM-internal terms, so if you guys want to roll the dice... but it seems like risk that is >= what we have with this bug patched.

/be

Comment 33

[Nicholas Nethercote [inactive]]

I don't care enough to have an argument about something this obscure, esp. this close to a release! :)  Let's leave it alone.

Comment 34

[Brendan Eich [:brendan]]

I talked to dvander, he made a stronger argument (a proof is just a convincing argument) that we don't need this bug's patch. But I agree with comment 33, we are pretty busy with more important work.

Followup bug?

/be

Comment 35

[Nicholas Nethercote [inactive]]

> Followup bug?

Bug 625647!