497015 - TM: Wrong value with upvars referring to top-level let variables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for each(let a in ['', 8888, 8888, /x/]) {
    '';
    for each(e in ['', false, '']) {
        (function(e) {
            for each(let c in ['']) {
                print(a);
            }
        })();
        for (let aa = 0; aa < 1; ++aa) {
            a = aa;
        }
    }
}

Note the extra "8888" in JIT'ed output. Tested on 191 revision 2d0f0efc8f14. Patch in bug 496922 comment 2 does not fix this.

===

$ ./js-dbg-moz191-intelmac 
js> for each(let a in ['', 8888, 8888, /x/]) {
    '';
    for each(e in ['', false, '']) {
        (function(e) {
            for each(let c in ['']) {
                print(a);
            }
        })();
        for (let aa = 0; aa < 1; ++aa) {
            a = aa;
        }
    }
}

0
0
8888
0
0
8888
0
0
/x/
0
0
0
js> ^C
$ ./js-dbg-moz191-intelmac -j
js> for each(let a in ['', 8888, 8888, /x/]) {
    '';
    for each(e in ['', false, '']) {
        (function(e) {
            for each(let c in ['']) {
                print(a);
            }
        })();
        for (let aa = 0; aa < 1; ++aa) {
            a = aa;
        }
    }
}

0
0
8888
0
0
8888
0
8888
/x/
0
0
0
js>

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(Not yet an optimal regression window)

No difference - http://hg.mozilla.org/tracemonkey/rev/0fb93c90d996
Difference present - http://hg.mozilla.org/tracemonkey/rev/d62fa90d2035

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=0fb93c90d996&tochange=d62fa90d2035

Comment 2

[David Mandelin [:dmandelin]]

This appears to be related to setting upvars defined with |let|. The function js_GetUpvarOnTrace is accessing the correct entry on the trace stack, but that entry has the wrong value--it has the value before the assignment |a=aa|.

Comment 3

[David Mandelin [:dmandelin]]

Since this is a blocker, I should add that it can be quick-fixed by aborting tracing of a JSOP_GETUPVAR when the target frame (containing uvpar definition) has no |fun| member inside TraceRecorder::upvar(). For now, I'll try to fix it so it doesn't have to abort.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

The |!fp->fun| case of upvar access was not being handled correctly. Specifically, if the argument to JSOP_GETUPVAR is |n|, then when reading from the interpreter, we should read from position |n+fp->fun->nfixed|. (This is defined by js_GetUpVar.) But if we are reading from the trace native stack, the callee, this, args, and locals are not pushed to the native stack, so we should read from position |n| of the slots area of the stack frame. I didn't realize earlier that the addressing needed to be different for the two cases.

The test case doesn't fail inside the regression test suite, so I can't really check in a test case. I have a personal directory of standalone files that I use as personal upvar regression tests, and I have added it there.

Comment 5

[David Mandelin [:dmandelin]]

Pushed to TM as bf1e2a11e031.

Comment 6

[Robert Sayre]

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

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/da574c09d790

Comment 9

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.