Closed Bug 495773 Opened 15 years ago Closed 15 years ago

TM: wrong number with nested anonymous functions, again

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: dmandelin)

Details

(Keywords: fixed1.9.1, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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?
Math is hard? Since the last one blocked, so does this.
Flags: blocking1.9.1? → blocking1.9.1+
Dave helped a ton and thereby painted a target on himself. Once more unto the breach and all that.

/be
Assignee: general → dmandelin
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.
Attachment #380901 - Flags: review?(gal)
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 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
Pushed to TM with nits fixed as eaaddd9ccd64.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/eaaddd9ccd64
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
js/tests/js1_8_1/regress/regress-495773.js
Flags: in-testsuite+
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: