465915 - TM: Wrong number with |let|, JSVAL_INT_MAX, type-unstable loop, global

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

for each (b in [0x3FFFFFFF, 0x3FFFFFFF, 0x3FFFFFFF]) {
  for each (let e in [{}, {}, {}, "", {}]) { 
    b = (b | 0x40000000) + 1;
    print('' + b); 
  }
}
delete b;

The last number printed is wrong with -j:

2147483648
-1073741823
-1073741822
-1073741821
-1073741820
2147483648
-1073741823
-1073741822
-1073741821
-1073741820
2147483648
-1073741823
-1073741822
-1073741821
-1073741695    <-- wrong (off by 125)

Comment 1

[Jeff Walden [:Waldo]]

I stepped through in a debugger which I left open overnight (and promptly lost when I accidentally turned off the switch powering the outlet where my laptop was plugged), and there seems to have been a missing (?) i2f somewhere, because the value passed to js_BoxInt32 (I *think* that was the builtin being called) was approximately $xmm0.v2_double[1] == -1073741656.000004, but $xmm0.v4_int32[3] == -1073741821 (or thereabouts, going from memory, last digit might have been 0).

Comment 2

[David Mandelin [:dmandelin]]

I'm pretty close on this, but I don't have time to figure out the last piece right now. What happens is that the generated code in the last trace run is trying to load the global 'b', and expects it to be a double, because the type map says so. But it's really an int. And that's because the second-to-last trace run stores an int there. The generated LIR:

              add eax,1                       eax(or1) ecx(gp) ebx($stack3) esi(sp) edi(cx)
              jo 0x292f79                     eax(add1) ecx(gp) ebx($stack3) esi(sp) edi(cx)
--------------------------------------- exit block (LIR_xt|LIR_xf)
        0x292f79:
                                              merging registers (intersect) with existing edge
              mov ecx,-4(ebp)                
              mov eax,2658520                
              mov esp,ebp                    
        0x292f83:
              jmp 0x291ff9                   
--------------------------------------- end exit block 0x289124
    i2f2 = i2f add1
    st gp[656] = add1
    ld8 = ld cx[152]
    ld9 = ld ld8[60]
    sti sp[40] = PCVAL_TO_OBJECT(pcval)
    sti sp[48] = ld9
    sti sp[56] = ATOM_TO_STRING(atom)
    sti sp[64] = add1
    js_NumberToString1 = js_NumberToString ( cx i2f2 )
    eq1 = eq js_NumberToString1, 0
    xt2: xt eq1 -> 0:100 sp+72 rp+0
              cvtsi2sd xmm0,eax               eax(add1) ecx(gp) ebx($stack3) esi(sp) edi(cx)
              mov 656(ecx),eax                eax(add1) ecx(gp) ebx($stack3) esi(sp) edi(cx) xmm0(i2f2)

At the very end you see that it converts the value (eax) to a double in xmm0, but then just stores the int eax anyway. (xmm0 is used later on down to convert to a string and print out, so it's not dead here even in this wrong code.) 

I don't know where the code is that does the bad thing. I thought it would be in record_SetPropHit because that's where JSOP_SETNAME seems to be handled, but in fact the debug printouts for this LIR don't seem to appear until later. But maybe that's due to a caching effect in the way debug output is generated.

Comment 3

[David Mandelin [:dmandelin]]

The problem seems to be in js_JoinPeersIfCompatible. This function links (via assm()->patch()) exit A from trace 1 to the header of trace 2 iff the stack typemap at exit A matches the stack typemap at trace 2 header. But this is wrong if the global typemaps don't match.

js_JoinPeersIfCompatible gets the stack typemap for trace 2 header from the TreeInfo. TreeInfo currently has no field for the global type map. Do we need to make TreeInfo hold the full type map? Please advise.

Comment 4

[Brendan Eich [:brendan]]

Mapping global types per tree is (IIUC) happening over in bug 469044. We need it for sure.

Dave, you willing to own this pending progress in bug 469044?

/be

Comment 5

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

(In reply to comment #3) 
> TreeInfo currently has no field for the global type map. Do we need
> to make TreeInfo hold the full type map? Please advise.

Yes, but in the meantime a global typemap change should be flushing the JIT cache.

Comment 6

[Andreas Gal :gal]

WFM in TM tip (blocking bug 469044 has been fixed).

Comment 7

[Andreas Gal :gal]

Added test case: http://hg.mozilla.org/tracemonkey/rev/92191eb6ded8

Comment 8

[Bob Clary [:bc] (inactive)]

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