TM: wrong number with >>>, various types of loops

RESOLVED DUPLICATE of bug 495958

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED DUPLICATE of bug 495958
9 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: gal)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla1.9.1
x86
Mac OS X
testcase
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dupe 495958])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
var v = 0;
for each (var a in [0, 1]) {
    print(v);
    for (var b = 0; b < 3; ++b) {
        v = a >>> 0;
        for each (var c in [{}, {}, {}, 1]) {
        }
    }
}

Without JIT:
0
0

With JIT:
0
-6.277435978499887e+66

I think this is a recent regression, since my fuzzer is hitting it a lot this morning.
(Assignee)

Comment 1

9 years ago
Crap. Can we bisect?
(Assignee)

Comment 2

9 years ago
Confirmed with TM tip. Not a happy day.
Flags: blocking1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 3

9 years ago
Regressed by:

changeset:   28910:daf5763d7da4
user:        Andreas Gal <gal@uci.edu>
date:        Tue Jun 02 22:28:59 2009 -0400
summary:     Bug 495897 - Assertion failure: ti->typeMap.matches(ti_other->typeMap) with undeclared global. r=dvander

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 4

9 years ago
My best guess here is that the inner loop gets specialized further and becomes incompatible with the loop that embeds it. It will take a little while to confirm this.

Updated

9 years ago
Assignee: general → gal
(Assignee)

Comment 5

9 years ago
graydon is helping too
(Assignee)

Comment 6

9 years ago
Created attachment 381411 [details] [diff] [review]
patch to flush out whats wrong (not a fix yet)
(Reporter)

Comment 7

9 years ago
var v = 0;
for each (var e in [1, 3, 1, 1]) {
    v += true;
    for each (var h in [4, 4, 4, {}, 4, {}]) {
        for (var a = 0; a < 1; ++a) {
        }
    }
}
print(v);

Without JIT: 4
With JIT:    -6.27743597849989e+66
(Reporter)

Comment 9

9 years ago
for each (var d in [1, 1, 1, 1]) {
    d >>>= 0;
    for (var c = 0; c < 2; ++c) {
        ("" + function () {});
    }
}
print(d);

Without JIT: 1
With JIT:    -6.2774359784998874e+66
(Assignee)

Comment 10

9 years ago
Slight simplification:

var c = 0;
for each (var d in [1, 1, 1, 1]) {
    d >>>= 0;
    for (var c = 0; c < 2; ++c) {
        +function () {};
    }
}
print(d);
(Assignee)

Comment 11

9 years ago
Created attachment 381443 [details] [diff] [review]
more wip, still borked
Attachment #381411 - Attachment is obsolete: true
Attachment #381435 - Attachment is obsolete: true
(Assignee)

Comment 12

9 years ago
Created attachment 381444 [details] [diff] [review]
even more propagation, of course still doesn't work
Attachment #381443 - Attachment is obsolete: true
(Assignee)

Comment 13

9 years ago
Created attachment 381446 [details] [diff] [review]
this propagates in every direction, still broken
Attachment #381444 - Attachment is obsolete: true
(Assignee)

Comment 14

9 years ago
I am pretty confident these are dups:

+[(e = {}, (function () e)()) for each (e in ["", {}, "", {}, ""])][4];

Crash [@ js_ValueToNumber] touching 0x20000000

+[(e = {}, (function () e)()) for each (e in ["", {}, "", {}, ""])];

Crash [@ JS_Enumerate] touching 0x20000000

https://bugzilla.mozilla.org/show_bug.cgi?id=496270

Might be easier to work with these.
Group: core-security
(Assignee)

Comment 15

9 years ago
Actually never mind #14. #14 looks like upvar on trace.
Created attachment 381588 [details]
a slightly informative log

Here's a log of the minimal run above. It's kinda nutty. There's no emitted treecall -- just a couple executed treecalls while recording the outer loop that BRANCH_EXIT -- and the only dependency that grows is between two differently type-specialized peers on the inner tree. The outer->inner call appears to be type compatible.

I'm perplexed, but maybe this helps shed light? I'll keep looking.
(Assignee)

Comment 17

9 years ago
Created attachment 381664 [details] [diff] [review]
also aggressively unspeculate on globals, not just stack slots
Attachment #381664 - Flags: review?
(Assignee)

Comment 18

9 years ago
I think we were digging in the wrong direction here. Look at this:

This is the last invocation of FlushNativeStackFrame. 

(gdb) x/8bx &np[83]
0xbfffcea0:	0xcd	0xcd	0xcd	0xcd	0xcd	0xcd	0xcd	0xcd
(gdb) p np[83]
$6 = -6.2774359784998874e+66
(gdb) p gslots[1]
$7 = 83
(gdb) p ngslots
$8 = 2
(gdb) 

We write back a value that was never imported or written to. It contains the original poison value (memset 0xcd).

This is not a tested tree.
(Assignee)

Comment 19

9 years ago
This is the last entry:

#0  0x00140796 in BuildNativeGlobalFrame (cx=0x30bc70, ngslots=2, gslots=0x30aea0, mp=0x30de52 "\001\001", '?' <repeats 12 times>, "\003", np=0xbfffc77c) at ../jstracer.cpp:1811

Types are int and int:

(gdb) x/2bx mp
0x30de52:	0x01	0x01

Object contains 1 and 1 in the 2 slots we are looking at:

  82 = 1
  83 = 1

We unpack those correctly:

(gdb) x/8bx &global[82]
0xbfffca0c:	0x01	0x00	0x00	0x00	0xcd	0xcd	0xcd	0xcd
(gdb) x/8bx &global[83]
0xbfffca14:	0x01	0x00	0x00	0x00	0xcd	0xcd	0xcd	0xcd
(gdb)
(Assignee)

Comment 20

9 years ago
(gdb) x/8bx &global[82]
0xbfffca0c:	0x01	0x00	0x00	0x00	0xcd	0xcd	0xcd	0xcd
(gdb) x/8bx &global[83]
0xbfffca14:	0x01	0x00	0x00	0x00	0xcd	0xcd	0xcd	0xcd
(gdb) n
4400	    u.code = f->code();
(gdb) n
4403	    state->startTime = rdtsc();
(gdb) n
4406	    JS_ASSERT(!tm->tracecx);
(gdb) n
4407	    tm->tracecx = cx;
(gdb) n
4408	    state->prev = cx->interpState;
(gdb) n
4409	    cx->interpState = state;
(gdb) 
4411	    debug_only(fflush(NULL);)
(gdb) 
4416	    rec = u.func(state, NULL);

We enter with int 1 and 1 in the slots.

We leave with the same state:

(gdb) x/8bx &global[83]
0xbfffca14:	0x01	0x00	0x00	0x00	0xcd	0xcd	0xcd	0xcd

However, when writing back we think its a double:

4621	    if (innermost->numGlobalSlots == ngslots) {
(gdb) 
4622	        globalTypeMap = getGlobalTypeMap(innermost);
(gdb) 
4644	                              stack, NULL);
(gdb) x/2bx globalTypeMap
0x2c58f5:	0x01	0x02
(gdb)
(Assignee)

Comment 21

9 years ago
global[83] contains an integer, and its the 2nd slot, and its 0x02 which means double. So we try to box back 0x01    0x00    0x00    0x00    0xcd    0xcd    0xcd    0xcd as a double, instead of 0x01    0x00    0x00    0x00    as an int.
(Assignee)

Comment 22

9 years ago
This is the last flush which causes he bad value to be read:

Breakpoint 1, FlushNativeGlobalFrame (cx=0x30bc70, ngslots=2, gslots=0x30aea0, mp=0x2c58f5 "\001\002", np=0xbfffc77c) at ../jstracer.cpp:1838
1838	    uint8* mp_base = mp;
(gdb) up
#1  0x001581ee in LeaveTree (state=@0xbfffc730, lr=0x2c58b4) at ../jstracer.cpp:4649
4649	    FlushNativeGlobalFrame(cx, ngslots, gslots, globalTypeMap, global);
(gdb) p lr->from->root
$7 = (class nanojit::Fragment *) 0x30dcd0
(gdb) p *lr
$8 = {
  <nanojit::SideExit> = {
    guards = 0x2c5914, 
    from = 0x30e370, 
    target = 0x0, 
    switchInfo = 0x0
  }, 
  members of VMSideExit: 
  block = 0x0, 
  pc = 0x1d03c2 "5", 
  imacpc = 0x30daad "#Q?", 
  sp_adj = 40, 
  rp_adj = 0, 
  calldepth = 0, 
  numGlobalSlots = 2, 
  numStackSlots = 5, 
  numStackSlotsBelowCurrentFrame = 0, 
  exitType = BRANCH_EXIT, 
  nativeCalleeWord = 0
}

(gdb) p lr
$9 = (VMSideExit *) 0x2c58b4

We are coming from fragment 0x30dcd0.

These are all recordings:

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x0, f=0x30dcd0, ti=0x30ddf0, stackSlots=2, ngslots=0, typeMap=0x30de50 "", expectedInnerExit=0x0, outer=0x0, outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x0, f=0x30dc20, ti=0x30e110, stackSlots=2, ngslots=1, typeMap=0x30dee0 "", expectedInnerExit=0x0, outer=0x0, outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x0, f=0x30df40, ti=0x30e110, stackSlots=2, ngslots=2, typeMap=0x30ddb0 "", expectedInnerExit=0x0, outer=0x30da94 "?h", outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x0, f=0x30dc20, ti=0x30e070, stackSlots=2, ngslots=2, typeMap=0x30ddc0 "", expectedInnerExit=0x0, outer=0x0, outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x27cc04, f=0x30e1d0, ti=0x30e110, stackSlots=5, ngslots=2, typeMap=0x27cc40 "", expectedInnerExit=0x0, outer=0x30da94 "?h", outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x27a3b4, f=0x30e370, ti=0x30ddf0, stackSlots=5, ngslots=0, typeMap=0x27a3f0 "", expectedInnerExit=0x0, outer=0x0, outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x0, f=0x30dc20, ti=0x30e270, stackSlots=2, ngslots=2, typeMap=0x30e4e0 "", expectedInnerExit=0x0, outer=0x0, outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c
Continuing.

Breakpoint 3, 0x0015c5b7 in js_StartRecorder (cx=0x30bc70, anchor=0x2c6414, f=0x30e540, ti=0x30e110, stackSlots=5, ngslots=2, typeMap=0x2c6450 "", expectedInnerExit=0x0, outer=0x30da94 "?h", outerArgc=0) at ../jstracer.cpp:3472
3472	                 VMSideExit* expectedInnerExit, jsbytecode* outer, uint32 outerArgc)
(gdb) c

So this is the first recording.
(Assignee)

Comment 23

9 years ago
Since its the first recording and we import lazily, there are initially no global slots. The two integer slots must have been gained later.

Question: why does the side exit know about 2 slots then? And if those were added lazily, why is there no associated read which would properly initialize the slot with a double?
(Assignee)

Comment 24

9 years ago
Ok, there is more than one trace in play here. This are unstable traces that are linked together. So we enter tree 1, and then fall out of the side exit for tree 2. tree 1 was recorded with just one global known and later probably grew a global but tree 2 always was recorded with 2 slots.
(Assignee)

Comment 25

9 years ago
This is with the latest patch applied, minus the EmitTreeCall stuff we put in:

Starting program: /Users/gal/workspace/tracemonkey-repository/js/src/Darwin_DBG.OBJ/js -j x.js
Reading symbols for shared libraries . done
Error in re-setting breakpoint 1:
Function "../jstracer.cpp:js_AttemptToExtendTree(JSContext*, VMSideExit*, VMSideExit*, unsigned char*)" not defined.

Breakpoint 2, js_AttemptToExtendTree (cx=0x30bc70, anchor=0x27cc04, exitedFrom=0x0, outer=0x30da94 "?h") at ../jstracer.cpp:4002
4002	                                ngslots, typeMap, exitedFrom, outer, cx->fp->argc);
(gdb) p ngslots
$6 = 2
(gdb) p anchor->from->root
$7 = (class nanojit::Fragment *) 0x30df40
(gdb) p anchor->from->root->first
$8 = (class nanojit::Fragment *) 0x30dcd0
(gdb) c
Continuing.

Breakpoint 2, js_AttemptToExtendTree (cx=0x30bc70, anchor=0x27a3b4, exitedFrom=0x0, outer=0x0) at ../jstracer.cpp:4002
4002	                                ngslots, typeMap, exitedFrom, outer, cx->fp->argc);
(gdb) p ngslots
$9 = 0
(gdb) p anchor->from->root
$10 = (class nanojit::Fragment *) 0x30dcd0
(gdb) 

As you can see we have 2 peer traces here, but the 2nd one has ngslots == 0. So its out of sync with its friend, and ends up being linked together and specialized wrong.
(Assignee)

Comment 26

9 years ago
Ok, the slot lists are actually correct. ngslots is wrong.

Reading symbols for shared libraries . done
Error in re-setting breakpoint 1:
Function "../jstracer.cpp:js_AttemptToExtendTree(JSContext*, VMSideExit*, VMSideExit*, unsigned char*)" not defined.

Breakpoint 2, js_AttemptToExtendTree (cx=0x30bc70, anchor=0x27cc04, exitedFrom=0x0, outer=0x30da94 "?h") at ../jstracer.cpp:4002
4002	                                ngslots, typeMap, exitedFrom, outer, cx->fp->argc);
(gdb) p anchor->from->root
$11 = (class nanojit::Fragment *) 0x30df40
(gdb) p *((TreeInfo*)anchor->from->root->vmprivate)->globalSlots
$12 = {
  <avmplus::GCObject> = {<No data fields>}, 
  members of Queue<short unsigned int>: 
  _data = 0x30aea0, 
  _len = 2, 
  _max = 16
}
(gdb) p *((TreeInfo*)anchor->from->root->first->vmprivate)->globalSlots
$13 = {
  <avmplus::GCObject> = {<No data fields>}, 
  members of Queue<short unsigned int>: 
  _data = 0x30aea0, 
  _len = 2, 
  _max = 16
}
(gdb) c
Continuing.

Breakpoint 2, js_AttemptToExtendTree (cx=0x30bc70, anchor=0x27a3b4, exitedFrom=0x0, outer=0x0) at ../jstracer.cpp:4002
4002	                                ngslots, typeMap, exitedFrom, outer, cx->fp->argc);
(gdb) p *((TreeInfo*)anchor->from->root->vmprivate)->globalSlots
$14 = {
  <avmplus::GCObject> = {<No data fields>}, 
  members of Queue<short unsigned int>: 
  _data = 0x30aea0, 
  _len = 2, 
  _max = 16
}
(gdb)
(Assignee)

Comment 27

9 years ago
This bug was the result of a bad patch for bug 495958, which we re-opened and fixed for real. With the new patch this bug no longer occurs. We should probably still add the test cases to trace-test though, just as future back stop against making the same mistake again.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 495958
(Assignee)

Comment 28

9 years ago
This bug should be opened up when it lands in branch. It was never broken in anything we shipped and is a recent regression only.
Group: core-security
Flags: wanted1.9.0.x-
Whiteboard: [sg:dupe 495958]
You need to log in before you can comment on or make changes to this bug.