Closed Bug 613692 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: philip, Assigned: bzbarsky)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

Attached patch test caseSplinter Review
Running with -j:

  for (var i = 0; i < 100; ++i) {
    var n = undefined;
    if (n++) { }
  }

Assertion failure: LIR type error (start of writer pipeline): arg 1 of 'eqd' is 'immi' which has type int (expected double): 0 (../nanojit/LIR.cpp:3207)
Here's the problematic code:

00008:   1  trace 0
    ------------------------------ # JSOP_TRACE

00011:   2  getgname "undefined"
    ------------------------------ # JSOP_GETGNAME
    globalObj = immi 0xf6c02028
[1] sti.sp sp[0] = strict/*0*/
    
00014:   2  setglobal "n"
    ------------------------------ # JSOP_SETGLOBAL
    $global1 = ldd.eos eos[1528]
    sti.eos eos[1528] = strict/*0*/
    
00018:   3  globalinc "n"
    ------------------------------ # JSOP_GLOBALINC
[2] immd1 = immd nan
    sti.sp sp[0] = strict/*0*/
    std.eos eos[1528] = immd1/*nan*/

00021:   3  ifeq 24 (3)
    ------------------------------ # JSOP_IFEQ
    immd2 = immd 0
**  eqd1 = eqd strict/*0*/, immd2/*0*/
    eqi2 = eqi eqd1, strict/*0*/
    eqd2 = eqd strict/*0*/, strict/*0*/
    andi1 = andi eqd2, eqi2
    eqi3 = eqi andi1, strict/*0*/
    eqi4 = eqi eqi3, strict/*0*/
    xf2: xf eqi3 -> exit=0x9302a44 pc=0x92ff989 imacpc=(nil) sp+8 rp+0 BRANCH
    

AFAICT the problem is that |undefined| gets imported by
TraceRecorder::importImpl() as an integer, using w.immiUndefined() (line
[1]).

Then TraceRecorder::incHelper() has this code:

    if (v.isUndefined()) {
        v_after = w.immd(js_NaN);

which evaluates the increment at record-time (line [2]).  The result (Nan)
is correct, but has a different LIR type -- double instead of int.  Then the
code that follows uses the slot info from importImpl() to determine the LIR 
type and we get a mismatch (line [3]).  The same thing happens if you replace 
|undefined| with |null|.

The problem seems to be the assumption that the record-time increment will
give a result with the same type.  Fixing that seems difficult, whereas
aborting if we increment |undefined| or |null| is easy and seems unlikely to
affect performance noticeably.  So I wrote a patch that does that.

However... the same issue affects strings (the test case in the patch still
asserts because of this).  But incHelper() doesn't return NaN for
incremented strings.  So I'm a bit confused by that.
Assignee: general → nnethercote
Status: NEW → ASSIGNED
Attachment #492253 - Flags: feedback?(gal)
bug 605858 just started tracing inc-on-undefined specifically for performance reasons on a 3rd-party benchmark, fwiw...
(In reply to comment #1)
> The problem seems to be the assumption that the record-time increment will
> give a result with the same type.

Cc'ing gal and dvander.

> However... the same issue affects strings (the test case in the patch still
> asserts because of this).  But incHelper() doesn't return NaN for
> incremented strings.  So I'm a bit confused by that.

js> x=""
""
js> ++x
1
js> y="1"
"1"
js> ++y
2
js> z="foo"
"foo"
js> ++z
NaN

Strings can convert to number not NaN.

/be
Yeah, this is a regression from bug 605858 and the undefined part is the part of that patch that really matters...
Blocks: 605858
And I guess I really should have written a trace-test for that, but we have jitstats disabled anyway, so there was no way to test it.  :(
So how do we handle type-changing sets?  JSOP_SETLOCAL seems to trace into a strict subset of the code that inc() generates...

In particular, why do things work if I replace |n++| with |n = (1/0)|?  That doesn't seem to have the |strict| thing going on...
I assume the fact that "strict" is involved at all is some sort of weird aliasing issue, btw (in the sense that the 0 there shouldn't really be named "strict")?
So for globalinc, we get this:

    immd1 = immd nan
    sti.sp sp[0] = strict/*0*/
    std.eos eos[1560] = immd1/*nan*/

for the 1/0 case, we get JSOP_DOUBLE and then JSOP_SETGLOBAL, like so:

    immd1 = immd inf
    std.sp sp[0] = immd1/*inf*/
    std.eos eos[1560] = immd1/*inf*/

and then the JSOP_IFEQ in that case is empty altogether (?) in stead of doing the eqd stuff that happens in the inc case.
So I don't understand why I don't see any eqd in the log in the 1/0 case...
Ah, looks like ins2 eliminates that because the args are double immediates.

In any case, the key seems to be that in the 1/0 case we stick a double in sp[0], while in the inc case we put an int there...  But that seems correct, since |n++| should test as |undefined| in that case.  If I replace it with |++n| then I get a double in sp[0].

One interesting thing: in the |inc| case, when we go to record IFEQ, in ifop we end up with |v| being NaN while v_ins is an immI.  The latter is correct.  The former is not, as far as I can tell; the value ifeq sees in interp should be undefined, not NaN.
OK. So what happens here is that the "do_int_fast_incop:" code in jsinterp sees that vp is undefined, so not int32, and proceeds to call js_DoIncDec.  It passes  &regs.sp[-1] as |vp| to js_DoIncDec, and passes its original vp as vp2.

Now js_DoIncDec for post-increment calls ValueToNumber on *vp and stores the resulting value in *vp (that is, the return value of doing ++ on |undefined| is actually NaN, not |undefined|.

The tracer needs to do something similar.  I clearly should have copy/pasted more of ::relational here!
Attachment #492668 - Flags: review?(dvander)
njn, stealing if you don't mind.
Assignee: nnethercote → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
blocking2.0: --- → beta9+
Attachment #492668 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/fe0e393e3530
Whiteboard: [need review] → fixed-in-tracemonkey
(In reply to comment #7)
> I assume the fact that "strict" is involved at all is some sort of weird
> aliasing issue, btw (in the sense that the 0 there shouldn't really be named
> "strict")?

Yes, the first zero immediate gets named "strict" and then all the ones that follow get CSE'd so they all end up being named "strict".  It is confusing.

(In reply to comment #13)
> njn, stealing if you don't mind.

Not at all.  It would be nice if the newly added test checked 'null' and maybe strings as well, though I see you've already landed it...
> It would be nice if the newly added test checked 'null' and maybe
> strings as well

Good idea.  I'll add those to the test.
Duplicate of this bug: 613152
http://hg.mozilla.org/mozilla-central/rev/fe0e393e3530
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 617617
No longer depends on: 617617
Attachment #492253 - Flags: feedback?(gal) → feedback+
Target Milestone: --- → mozilla2.0b8
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
No longer blocks: 605858
blocking2.0: beta9+ → betaN+
Blocks: 605858
You need to log in before you can comment on or make changes to this bug.