Closed
Bug 495958
Opened 12 years ago
Closed 12 years ago
"Assertion failure: isInt32(*p)" with type-unstable loops and undeclared global
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: gal)
References
Details
(4 keywords)
Attachments
(1 file, 7 obsolete files)
819 bytes,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
// b must be undeclared global? for each (var a in [new Number(1), new Number(1), {}, {}, new Number(1)]) { for each (b in [2, "", 2, "", "", ""]) { for each (var c in [{}, {}, 3, 3, 3, 3, {}, {}]) { 4 + a; } } } Assertion failure: isInt32(*p), at ../jstracer.cpp:2008 Before I reduced it, the assertion was different: Assertion failure: isNumber(*p) == (t == JSVAL_DOUBLE), at ../jstracer.cpp:2016
![]() |
||
Comment 1•12 years ago
|
||
autoBisect shows this is probably related to bug 479110 : The first bad revision is: changeset: 26780:790c0eda3122 user: Andreas Gal date: Sat Apr 04 17:23:34 2009 -0400 summary: Bug 479110 - TM: avoid frequent mismatch exits. r=brendan
Comment 2•12 years ago
|
||
Our of curiousity, if this regressed in April, why are we just finding it now, in June? Jesse, are your fuzzers only just getting to this point ..?
![]() |
||
Comment 3•12 years ago
|
||
(In reply to comment #2) > Our of curiousity, if this regressed in April, why are we just finding it now, > in June? Jesse, are your fuzzers only just getting to this point ..? Beltzner: fuzzers find bugs randomly, and how often the bugs are found depends on various factors that can be unpredictable. I have occasionally found fuzzer bugs that show up many weeks/months after the offending patch landed, even though machines have been fuzzing continuously 24/7. I won't be surprised if fuzzers continue to find assertions / crashes _after_ 3.5 is released.
Reporter | ||
Comment 4•12 years ago
|
||
Not sure why I haven't hit this specific bug before. I haven't changed this fuzzer lately, and I have run it for a decent number hours. My guess is that this bug just requires a fairly specific and complicated testcase.
Updated•12 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Updated•12 years ago
|
Whiteboard: [needs assignee]
Updated•12 years ago
|
Assignee: general → jorendorff
Assignee | ||
Comment 5•12 years ago
|
||
This might be a dup of the bug I am working on, but it can't hurt if we look at the two cases in parallel.
Whiteboard: [needs assignee]
Assignee | ||
Comment 6•12 years ago
|
||
Not a dup, unfortunately.
Assignee | ||
Comment 7•12 years ago
|
||
The blamed changeset is a red herring. We just didn't trace all of the loop before, now we do.
Assignee | ||
Comment 8•12 years ago
|
||
We are extending a tree, but the typemap of the side exit isn't compatible with the current VM state. (gdb) bt #0 JS_Assert (s=0x1bebe4 "isInt32(*p)", file=0x1bd8a9 "../jstracer.cpp", ln=2031) at ../jsutil.cpp:69 #1 0x0013cee0 in TraceRecorder::import (this=0x313ff0, base=0x42fa54, offset=820, p=0x811764, t=1 '\001', prefix=0x1a7aba "global", index=2, fp=0x0) at ../jstracer.cpp:2031 #2 0x001541d0 in TraceRecorder::import (this=0x313ff0, treeInfo=0x312520, sp=0x42fa74, stackSlots=7, ngslots=3, callDepth=0, typeMap=0x313e20 "") at ../jstracer.cpp:2139 #3 0x001550f8 in TraceRecorder::TraceRecorder (this=0x313ff0, cx=0x30bc70, _anchor=0x438124, _fragment=0x313c60, ti=0x312520, stackSlots=7, ngslots=2, typeMap=0x313e20 "", innermostNestedGuard=0x2de834, outer=0x0, outerArgc=0) at ../jstracer.cpp:1410 #4 0x0015b783 in js_StartRecorder (cx=0x30bc70, anchor=0x438124, f=0x313c60, ti=0x312520, stackSlots=7, ngslots=2, typeMap=0x313e20 "", expectedInnerExit=0x2de834, outer=0x0, outerArgc=0) at ../jstracer.cpp:3511 #5 0x0015bab5 in js_AttemptToExtendTree (cx=0x30bc70, anchor=0x438124, exitedFrom=0x2de834, outer=0x0) at ../jstracer.cpp:4017 #6 0x00161548 in js_MonitorLoopEdge (cx=0x30bc70, inlineCallCount=@0xbffff278) at ../jstracer.cpp:4802 #7 0x0006fd2e in js_Interpret (cx=0x30bc70) at ../jsinterp.cpp:3300 #8 0x0009a345 in js_Execute (cx=0x30bc70, chain=0x2bd000, script=0x30d9f0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1622 #9 0x00010588 in JS_ExecuteScript (cx=0x30bc70, obj=0x2bd000, script=0x30d9f0, rval=0x0) at ../jsapi.cpp:5046 #10 0x00009a60 in Process (cx=0x30bc70, obj=0x2bd000, filename=0xbffff98c "x.js", forceTTY=0) at ../../shell/js.cpp:412 #11 0x0000a776 in ProcessArgs (cx=0x30bc70, obj=0x2bd000, argv=0xbffff88c, argc=2) at ../../shell/js.cpp:806 #12 0x0000ab43 in main (argc=2, argv=0xbffff88c, envp=0xbffff898) at ../../shell/js.cpp:4729
Assignee: jorendorff → gal
Assignee | ||
Comment 9•12 years ago
|
||
This is slightly simplified. It doesn't look a lot simpler but it generates less code. function boom() { var f = function(){}; var g = function(){}; g.prototype.__proto__ = {}; iq(f); iq(f); iq(f); iq(f); iq(g); } function iq(obj) { for (var i = 0; i < 10; ++i) "" + obj["prototype"]; } boom();
Assignee | ||
Comment 10•12 years ago
|
||
(#9 was wrong the wrong bug)
![]() |
||
Comment 11•12 years ago
|
||
(In reply to comment #9) > This is slightly simplified. It doesn't look a lot simpler but it generates > less code. > > function boom() > { > var f = function(){}; > var g = function(){}; > g.prototype.__proto__ = {}; > iq(f); > iq(f); > iq(f); > iq(f); > iq(g); > } > > function iq(obj) > { > for (var i = 0; i < 10; ++i) > "" + obj["prototype"]; > } > > boom(); Meant for bug 496054 ?
Assignee | ||
Comment 12•12 years ago
|
||
Ok, so here is what happens: (gdb) list 1405 /* If we came from exit, we might not have enough global types. */ 1406 if (ti->globalSlots->length() > ti->nGlobalTypes()) 1407 specializeTreesToMissingGlobals(cx, ti); 1408 1409 /* read into registers all values on the stack and all globals we know so far */ 1410 import(treeInfo, lirbuf->sp, stackSlots, ngslots, callDepth, typeMap); 1411 1412 if (fragment == fragment->root) { 1413 /* 1414 * We poll the operation callback request flag. It is updated asynchronously whenever (gdb) p ti->nGlobalTypes() $1 = 3 (gdb) p ti->globalSlots->length() $2 = 3 (gdb) p ngslots $3 = 2 We are extending a side exit. The original side exit knows only about 2 globals. The tree has grown another one, which has been added to the tree entry type map. We then call import() to read all the values currently alive. Since there is a missing type in the map, import completes the typemap: uint8* globalTypeMap = typeMap + stackSlots; unsigned length = treeInfo->nGlobalTypes(); /* * This is potentially the typemap of the side exit and thus shorter than the tree's * global type map. */ if (ngslots < length) { mergeTypeMaps(&globalTypeMap/*out param*/, &ngslots/*out param*/, treeInfo->globalTypeMap(), length, (uint8*)alloca(sizeof(uint8) * length)); } JS_ASSERT(ngslots == treeInfo->nGlobalTypes()); It uses the entry type map to do so. This is correct when recording a fresh tree, but its bogus when extending a tree. The extended tree might simply have different types for global slots at this location. So we copy a bogus type from the entry map into this location, recorder and interpreter disagree, hilarity ensues. Fix: import() is used during the initial importing, but also in side exit continuations. We have to move the typemap merging code out of import. When extending side exits, we have to capture the actual types instead of using the entry type map of the tree to complete the type map. More tomorrow.
Assignee | ||
Comment 13•12 years ago
|
||
Putting bug back in the pool until I wake up again.
Assignee: gal → general
Priority: -- → P1
Comment 14•12 years ago
|
||
I'm curious about something else here. Early in the debug spew, I get this: leaving trace at crasher.js:11@201, op=loop, lr=0xb7c64254, exitType=8, sp=6, calldepth=0, cycles=132885 stack0=object<0x83bf300:Iterator> stack1=object<0x83bf140:Number> stack2=object<0x83bf320:Iterator> stack3=int<2> stack4=object<0x83bf340:Iterator> stack5=double<3> Now double<3> here is not inherently wrong, because the jsval in question is an INT jsval being copied to a native slot of type double. But how did this trace ever get recorded? stack5 is never a double jsval in the interpreter.
Priority: P1 → --
Assignee | ||
Comment 15•12 years ago
|
||
Assignee: general → gal
Assignee | ||
Comment 16•12 years ago
|
||
#14: we always read doubles from arrays at the moment when we read a number. I have a patch to fix that, but its often not a speedup either. Specializing to int when reading from arrays makes crypto ridiculously slow since the tree explodes as it uses the 32nd bit all the time.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #381346 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #381350 -
Attachment is obsolete: true
Attachment #381351 -
Flags: review?(graydon)
Updated•12 years ago
|
Attachment #381351 -
Flags: review?(graydon) → review+
Comment 19•12 years ago
|
||
Comment on attachment 381351 [details] [diff] [review] patch, working Looks good, though we ought to clean Queue up a ways. I'll file a separate patch.
Assignee | ||
Comment 20•12 years ago
|
||
This patch didn't go through the try server. Feeling lucky and landing on TM directly. http://hg.mozilla.org/tracemonkey/rev/2ad658e9f42a
Whiteboard: fixed-in-tracemonkey
![]() |
||
Updated•12 years ago
|
Attachment #381351 -
Flags: review+
Comment 21•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2ad658e9f42a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 22•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5bb3a53e5ddd
Keywords: fixed1.9.1
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 23•12 years ago
|
||
The analysis and the patch are wrong. This causes bug 496185. The real bug here is as follows: #3 0x00156084 in TraceRecorder::TraceRecorder (this=0x313ff0, cx=0x30bc70, _anchor=0x458124, _fragment=0x313c60, ti=0x312520, stackSlots=7, ngslots=2, typeMap=0x313e20 "", innermostNestedGuard=0x2df834, outer=0x0, outerArgc=0) at ../jstracer.cpp:1410 1410 import(treeInfo, lirbuf->sp, stackSlots, ngslots, callDepth, typeMap); (gdb) p ngslots $11 = 2 (gdb) p *anchor $12 = { <nanojit::SideExit> = { guards = 0x458184, from = 0x312350, target = 0x313c60, switchInfo = 0x0 }, members of VMSideExit: block = 0x0, pc = 0x30db08 "\b??ML\b??ML\b??Mû\003?\001g\004?`\004?`\004\027\a{4", imacpc = 0x0, sp_adj = 56, rp_adj = 0, calldepth = 0, numGlobalSlots = 3, numStackSlots = 7, numStackSlotsBelowCurrentFrame = 0, exitType = NESTED_EXIT, nativeCalleeWord = 0 } We import with ngslots == 2, but in reality the anchor has 3 slots and it knows its a string. We merge in an int from the entry, which causes the assert die on.
Assignee | ||
Comment 24•12 years ago
|
||
And the real bug is ... Starting program: /Users/gal/workspace/tracemonkey-repository/js/src/Darwin_DBG.OBJ/js -j x.js Reading symbols for shared libraries . done Breakpoint 1, js_AttemptToExtendTree (cx=0x30bc70, anchor=0x458124, exitedFrom=0x2e1834, outer=0x0) at ../jstracer.cpp:4009 4009 fullMap.add(getStackTypeMap(e1), e1->numStackSlotsBelowCurrentFrame); (gdb) p *e1 $1 = { <nanojit::SideExit> = { guards = 0x458184, from = 0x312350, target = 0x313c60, switchInfo = 0x0 }, members of VMSideExit: block = 0x0, pc = 0x30db08 "\b??ML\b??ML\b??Mû\003?\001g\004?`\004?`\004\027\a{4", imacpc = 0x0, sp_adj = 56, rp_adj = 0, calldepth = 0, numGlobalSlots = 3, numStackSlots = 7, numStackSlotsBelowCurrentFrame = 0, exitType = NESTED_EXIT, nativeCalleeWord = 0 } (gdb) p *e2 $2 = { <nanojit::SideExit> = { guards = 0x2e1894, from = 0x30f030, target = 0x0, switchInfo = 0x0 }, members of VMSideExit: block = 0x0, pc = 0x30db08 "\b??ML\b??ML\b??Mû\003?\001g\004?`\004?`\004\027\a{4", imacpc = 0x0, sp_adj = 56, rp_adj = 0, calldepth = 0, numGlobalSlots = 2, numStackSlots = 7, numStackSlotsBelowCurrentFrame = 0, exitType = LOOP_EXIT, nativeCalleeWord = 0 } (gdb) We merge the type maps e1 and e2 and take e2's globals, but e2 happens to know about fewer globals (2), so we merge from the entry (giving us an int) instead of using the exit state, which is string. VMSideExit* e1 = anchor; VMSideExit* e2 = exitedFrom; fullMap.add(getStackTypeMap(e1), e1->numStackSlotsBelowCurrentFrame); fullMap.add(getStackTypeMap(e2), e2->numStackSlots); stackSlots = fullMap.length(); fullMap.add(getGlobalTypeMap(e2), e2->numGlobalSlots); ngslots = e2->numGlobalSlots; typeMap = fullMap.data();
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #381351 -
Attachment is obsolete: true
Attachment #381690 -
Flags: review?
![]() |
||
Updated•12 years ago
|
Attachment #381690 -
Flags: review? → review+
Assignee | ||
Comment 27•12 years ago
|
||
Pushed to TM. http://hg.mozilla.org/tracemonkey/rev/ea72bb61b701
Comment 28•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ea72bb61b701
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: fixed1.9.1
Comment 29•12 years ago
|
||
I have removed the fixed1.9.1 keyword due to the missing checkin of the followup patch.
Assignee | ||
Comment 30•12 years ago
|
||
The original patch was backed out of TM and has to be removed from m-c and branch as well. Re-opening until thats confirmed. http://hg.mozilla.org/tracemonkey/rev/47ae3c6dcf9e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 31•12 years ago
|
||
backout on mozilla-central last night: http://hg.mozilla.org/mozilla-central/rev/47ae3c6dcf9e backout on 191 today: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bb49f28e12ad
Comment 32•12 years ago
|
||
final fix: http://hg.mozilla.org/mozilla-central/rev/ea72bb61b701 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a4629227dcc0
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 34•12 years ago
|
||
Switching e2 and e1 causes this case to fail: for each(c in [new Boolean(true), new Number(), new String, new Boolean(true), new String(), new Boolean(true), new Boolean(true), new Number()]) { for each(a in [new String(), new String]) { print(c += (0 > c)) } } Switching it back makes it pass. So we have to resolve the incompatibility here. Investigating.
It can happen that the innermost guard exits with different types than the outermost guard. In this case we should use all of the innermost types before using the missing outer types.
Attachment #381690 -
Attachment is obsolete: true
Assignee | ||
Comment 36•12 years ago
|
||
Comment on attachment 381912 [details] [diff] [review] merge two typemaps the two global typemaps might have incompatible ordering. we have to be cautious how we merge them.
Attachment #381912 -
Flags: review-
Attachment #381912 -
Attachment is obsolete: true
Assignee | ||
Comment 38•12 years ago
|
||
Comment on attachment 381916 [details] [diff] [review] orders >diff -r 42531a24b8c0 js/src/jstracer.cpp >--- a/js/src/jstracer.cpp Fri Jun 05 13:55:51 2009 -0700 >+++ b/js/src/jstracer.cpp Sat Jun 06 02:22:10 2009 -0700 >@@ -4009,8 +4009,28 @@ > fullMap.add(getStackTypeMap(e1), e1->numStackSlotsBelowCurrentFrame); > fullMap.add(getStackTypeMap(e2), e2->numStackSlots); > stackSlots = fullMap.length(); >+ /* Get initial globals */ >+ TreeInfo* ti_outer = (TreeInfo*)f->vmprivate; >+ TreeInfo* ti_inner = (TreeInfo*)e2->from->root->vmprivate; > fullMap.add(getGlobalTypeMap(e1), e1->numGlobalSlots); >- ngslots = e1->numGlobalSlots; >+ fullMap.captureMissingGlobalTypes(cx, *ti_outer->globalSlots, stackSlots); >+ /* For each slot in e2 not in e1, add to e2's slotlist */ use inner and outer instead of e2 and e1. >+ for (uint32 i = 0; i < ti_inner->globalSlots->length(); i++) { >+ uint16 slot = ti_inner->globalSlots->data()[i]; >+ if (ti_outer->globalSlots->contains(slot)) >+ continue; At this point we know the slot is missing in the outer, and we know the index in outer, so we could just grab the type from there and add it to inner? (append) >+ ti_outer->globalSlots->add(slot); >+ } >+ /* Merge e2's types in */ >+ uint32 pos; >+ uint32 limit = JS_MIN(e2->numGlobalSlots, ti_inner->globalSlots->length()); >+ for (uint32 i = 0; i < limit; i++) { >+ uint16 slot = ti_inner->globalSlots->data()[i]; >+ assert(ti_outer->globalSlots->contains(slot)); >+ ti_outer->globalSlots->find(slot, &pos); >+ fullMap.data()[stackSlots + pos] = *(getGlobalTypeMap(e2) + i); >+ } >+ ngslots = ti_outer->globalSlots->length(); > typeMap = fullMap.data(); > } > JS_ASSERT(ngslots >= anchor->numGlobalSlots); >diff -r 42531a24b8c0 js/src/jstracer.h >--- a/js/src/jstracer.h Fri Jun 05 13:55:51 2009 -0700 >+++ b/js/src/jstracer.h Sat Jun 06 02:22:10 2009 -0700 >@@ -88,6 +88,16 @@ > return false; > } > >+ bool find(T a, unsigned* pos) { >+ for (unsigned n = 0; n < _len; ++n) { >+ if (_data[n] == a) { >+ *pos = n; >+ return true; >+ } >+ } >+ return false; >+ } >+ > void add(T a) { > ensure(_len + 1); > JS_ASSERT(_len <= _max); Pretty close. Needs a little bit more work. Thanks for helping with this.
Assignee | ||
Comment 39•12 years ago
|
||
Attachment #381916 -
Attachment is obsolete: true
Attachment #381967 -
Attachment is obsolete: true
Attachment #381969 -
Flags: review?(gal)
Assignee | ||
Updated•12 years ago
|
Attachment #381969 -
Flags: review?(gal) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
If there's a fix on m-c, isn't this bug FIXED? Would be clearer to have another bug for follow-on work (esp for those of us tracking blockers).
Assignee | ||
Comment 42•12 years ago
|
||
Fine by my either way. sayrer we land the patch. maybe jesse and gary could do some additional fuzzing to make sure we fixed it this time for real.
Assignee | ||
Comment 43•12 years ago
|
||
Comment on attachment 381912 [details] [diff] [review] merge two typemaps The slot list is shared between related trees, so this should work after all.
Attachment #381912 -
Flags: review- → review+
Assignee | ||
Updated•12 years ago
|
Attachment #381969 -
Flags: review+ → review-
Assignee | ||
Updated•12 years ago
|
Attachment #381912 -
Attachment is obsolete: false
Assignee | ||
Updated•12 years ago
|
Attachment #381969 -
Flags: review- → review+
Assignee | ||
Comment 44•12 years ago
|
||
Comment on attachment 381969 [details] [diff] [review] less confusing names Order is fixed. No need for this more complicated patch.
Attachment #381969 -
Attachment is obsolete: true
Attachment #381969 -
Flags: review+ → review-
Assignee | ||
Comment 45•12 years ago
|
||
Followup patch is in bug 496743.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Flags: wanted1.9.0.x-
You need to log in
before you can comment on or make changes to this bug.
Description
•