613692 - 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)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Philip Taylor]

Attachment: test case

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)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: partial patch (against TM 57773:da7fd699ddcb)

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.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

bug 605858 just started tracing inc-on-undefined specifically for performance reasons on a 3rd-party benchmark, fwiw...

Comment 3

[Brendan Eich [:brendan]]

(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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, this is a regression from bug 605858 and the undefined part is the part of that patch that really matters...

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

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...

Comment 7

[Boris Zbarsky [:bzbarsky]]

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")?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I don't understand why I don't see any eqd in the log in the 1/0 case...

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to update what our current value is when doing type conversions inside incHelper.

Comment 13

[Boris Zbarsky [:bzbarsky]]

njn, stealing if you don't mind.

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/tracemonkey/rev/fe0e393e3530

Comment 15

[Nicholas Nethercote [inactive]]

(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...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/d3adb226abe7 with more tests.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/fe0e393e3530

Comment 20

[LegNeato]

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)