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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: jandem, Assigned: njn)

Tracking

({assertion, regression, testcase})

unspecified
x86
Windows XP
assertion, regression, testcase
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: hardblocker, [sg:dupe 624547]-ish, fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
---
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)
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?

Comment 1

8 years ago
Feel free to assign this to me if it gets marked blocking.
Group: core-security
As a precaution, LIR type errors are generally filed security sensitive.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
BTW, as well as !==, the type errors also occur with !=, ==, ===.
(Assignee)

Comment 5

8 years ago
The fact that the ltag is JSVAL_TYPE_UNDEFINED seems wrong.
Is this fairly easy to fix?
(Assignee)

Comment 7

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

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

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Luke, CCing you as you touched record_JSOP_ARGSUB recently.
(Assignee)

Comment 10

8 years ago
Created attachment 499470 [details] [diff] [review]
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?
Assignee: general → nnethercote
Status: NEW → ASSIGNED
Attachment #499470 - Flags: review?(brendan)
Do we need to worry about cases where the object DELELEM happens on is not arguments at record time but is at runtime?
(Assignee)

Comment 12

8 years ago
(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.)
(Assignee)

Comment 13

8 years ago
This is a regression, DELELEM wasn't traced in 1.9.2.
Keywords: regression
blocking2.0: ? → betaN+
Created attachment 499922 [details] [diff] [review]
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
Attachment #499470 - Attachment is obsolete: true
Attachment #499922 - Flags: review?
Attachment #499470 - Flags: review?(brendan)
Attachment #499922 - Flags: review?
(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
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
Whiteboard: hardblocker
(Assignee)

Comment 17

8 years ago
> 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.
(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
(Assignee)

Comment 19

8 years ago
(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.
(Assignee)

Comment 20

8 years ago
In case that's not clear, I think my original patch is sufficient.
(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
OS: Mac OS X → Windows XP

Updated

8 years ago
Whiteboard: hardblocker → hardblocker, [sg:critical?]
(Assignee)

Comment 22

8 years ago
Created attachment 501218 [details] [diff] [review]
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.
Attachment #499922 - Attachment is obsolete: true
Attachment #501218 - Flags: review?(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
Attachment #501218 - Flags: review?(brendan) → review+
(Assignee)

Comment 24

8 years ago
http://hg.mozilla.org/tracemonkey/rev/9df93a2a40e5
Whiteboard: hardblocker, [sg:critical?] → hardblocker, [sg:critical?], fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/9df93a2a40e5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

8 years ago
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?
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!
(Assignee)

Comment 29

8 years ago
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?
(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
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.
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
(Assignee)

Comment 33

8 years ago
I don't care enough to have an argument about something this obscure, esp. this close to a release! :)  Let's leave it alone.
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
(Assignee)

Comment 35

8 years ago
> Followup bug?

Bug 625647!
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: hardblocker, [sg:critical?], fixed-in-tracemonkey → hardblocker, [sg:moderate], fixed-in-tracemonkey
Whiteboard: hardblocker, [sg:moderate], fixed-in-tracemonkey → hardblocker, [sg:moderate], fixed-in-tracemonkey, also fixed by 624547
Whiteboard: hardblocker, [sg:moderate], fixed-in-tracemonkey, also fixed by 624547 → hardblocker, [sg:dupe 624547]-ish, fixed-in-tracemonkey
Group: core-security
You need to log in before you can comment on or make changes to this bug.