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.

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

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!

njn, stealing if you don't mind.

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

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

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)