Closed
Bug 495773
Opened 15 years ago
Closed 15 years ago
TM: wrong number with nested anonymous functions, again
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jruderman, Assigned: dmandelin)
Details
(Keywords: fixed1.9.1, testcase, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
11.07 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
var q = []; for (var a = 0; a < 3; ++a) { (function () { for (var b = 0; b < 2; ++b) { (function () { for (var c = 0; c < 1; ++c) { q.push(b); } })(); } })(); } print(q.join("")); Result: 010101 without jit (correct) 010100 with jit (last digit incorrect) This bug exists on TM branch and 1.9.1 branch. This seems similar to bug 495566, which was fixed two days ago.
Flags: blocking1.9.1?
Comment 1•15 years ago
|
||
Math is hard? Since the last one blocked, so does this.
Flags: blocking1.9.1? → blocking1.9.1+
Comment 2•15 years ago
|
||
Dave helped a ton and thereby painted a target on himself. Once more unto the breach and all that. /be
Assignee: general → dmandelin
Assignee | ||
Comment 3•15 years ago
|
||
The problem seems to occur when we execute the trace for the middle loop, which calls the inner loop. At the point of the upvar access, the logical call stack is this: function encoded as locals stored in top-level interp JSStackFrame interp JSStackFrame outer lambda interp JSStackFrame outer trace native stack inner lambda outer trace CallInfo inner trace native stack The middle row is the problem: because there is no CallInfo for this function's activation, js_GetUpvarOnTrace doesn't look to see if the values are in a native frame, so it ends up calling js_GetUpvar. But that's not the location of the current value of |b| after it is mutated on line 4, so we get an old value. I think the solution is simply to check if the function in the current cx->fp (representing the active function on trace entry) has the upvar after searching the native stack and before calling js_GetUpvar. Once again, thanks for the fuzzer coverage! This stuff is hard to test.
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #380901 -
Flags: review?(gal)
Comment 5•15 years ago
|
||
Comment on attachment 380901 [details] [diff] [review] Patch with regression test "FinishGetUpvarOnTrace" is a pretty horrible name, otherwise looks great.
Attachment #380901 -
Flags: review?(gal) → review+
Comment 6•15 years ago
|
||
Comment on attachment 380901 [details] [diff] [review] Patch with regression test >diff -r 7ec985e33884 js/src/jstracer.cpp >--- a/js/src/jstracer.cpp Fri May 29 23:17:50 2009 -0700 >+++ b/js/src/jstracer.cpp Mon Jun 01 13:33:58 2009 -0700 >@@ -1831,16 +1831,29 @@ FlushNativeGlobalFrame(JSContext* cx, un > NativeToValue(cx, *vp, *mp, np + gslots[n]); > ++mp; > ); > debug_only_v(nj_dprintf("\n");) > return mp - mp_base; > } > > /* >+ * Helper for js_GetUpvarOnTrace. >+ */ >+static uint32 FinishGetUpvarOnTrace(InterpState* state, uint32 cookie, Nits: break line after type to start function name in column 1; no spaces at ends of lines! The name mixes a new "Finish" metaphor, where we use that to mean "finish using", especially for LIFO data structures. How about GetUpvarOnTraceTail? >+ uint32 nativeStackFramePos, uint8* typemap, >+ double* result) >+{ >+ uintN slot = UPVAR_FRAME_SLOT(cookie); Nit: blank line here. >+ if (cx->fp->fun && cx->fp->fun->u.i.script->staticLevel == upvarLevel) >+ return FinishGetUpvarOnTrace(state, cookie, 0, >+ state->outermostTree->stackTypeMap(), >+ result); Nit: brace consequent if it or the if-condition or any else clause are multiline. /be
Assignee | ||
Comment 7•15 years ago
|
||
Pushed to TM with nits fixed as eaaddd9ccd64.
Whiteboard: fixed-in-tracemonkey
Comment 8•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/eaaddd9ccd64
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 9•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/71c7ae28fa74
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•