507053 - TM: Incorrect output with traced JSOP_BINDNAME

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

Test case:

var f = function() {
  var p = 1;
  
  function g() {
    for (var i = 0; i < 5; ++i) {
      p = p * 2;
      print(p);
    }
  }
  
  g();
  
  print(p);
}

for (var i = 0; i < 5; ++i) {
  f();
  print();
}

The problem is that the traced JSOP_BINDNAME assumes that the Call object is fixed on trace but that's not true for successive calls to this trace.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 391248 [details] [diff] [review]
Patch

>diff -r 075b80a1cf6d js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp	Tue Jul 28 18:51:35 2009 -0400
>+++ b/js/src/jstracer.cpp	Tue Jul 28 17:51:34 2009 -0700
>@@ -11090,14 +11090,17 @@
>         }
>     }
> 
>-    /*
>-     * If obj is a js_CallClass object, then we are tracing a reference to an
>-     * upvar in a heavyweight function. We cannot reach this point of the trace
>-     * with a different call object because of the guard on the function call,
>-     * so we can assume the result of the bindname is constant on this trace.
>-     */
>-    if (obj != globalObj && OBJ_GET_CLASS(cx, obj) != &js_CallClass)
>-        ABORT_TRACE("Can only trace JSOP_BINDNAME with global or call object");
>+    if (obj != globalObj) {
>+        if (OBJ_GET_CLASS(cx, obj) != &js_CallClass)
>+            ABORT_TRACE("Can only trace JSOP_BINDNAME with global or call object");

Nit: blank line here would be easier on the eyes.

>+        /*
>+         * The interpreter version of JSOP_BINDNAME does the full lookup. We
>+         * don't need to do that on trace because we will leave trace if the
>+         * scope ever changes, so the result of the lookup cannot change.
>+         */
>+        stack(0, stobj_get_parent(get(&cx->fp->argv[-2])));

Is this assuming that the callee function's definition or expression was evaluated in the same Call object that is referenced by obj? That does not seem guaranteed.

/be

Comment 3

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> >+        /*
> >+         * The interpreter version of JSOP_BINDNAME does the full lookup. We
> >+         * don't need to do that on trace because we will leave trace if the
> >+         * scope ever changes, so the result of the lookup cannot change.
> >+         */
> >+        stack(0, stobj_get_parent(get(&cx->fp->argv[-2])));
> 
> Is this assuming that the callee function's definition or expression was
> evaluated in the same Call object that is referenced by obj? That does not 
> seem guaranteed.

The code above is simply an on-trace version of the code this method uses to get to |obj|, namely:

        obj = OBJ_GET_PARENT(cx, fp->callee);

I think this is right based on the following reasoning, although there are several assumptions I'm not entirely sure about:

- Lookup for JSOP_NAME-type operators should start at fp->scopeChain.
- If fp->callee is not heavyweight, then fp->scopeChain == OBJ_GET_PARENT(cx, fp->callee).
- record_JSOP_BINDNAME aborts if fp->callee is heavyweight.
- Therefore, lookup can start at OBJ_GET_PARENT(cx, fp->callee) on trace.

Comment 4

[Brendan Eich [:brendan]]

Assert harder to cover the assumptions with some DEBUG-built checking?

/be

Comment 5

[David Mandelin [:dmandelin]]

Attachment: Patch 2

Added an assert.

Comment 6

[Andreas Gal :gal]

Some new lines around stobj_get_paren might be nice (does the code around do it?).

Comment 7

[David Mandelin [:dmandelin]]

Pushed to TM with extra newlines (agreed it looks better) as be4280409fb3.

Comment 9

[Robert Sayre]

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

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 11

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090911 Namoroka/3.6a2pre and equivalent Mac build

Comment 12

[Bob Clary [:bc] (inactive)]

js/tests/js1_8_1/regress/regress-507053.js
v 1.9.3, 1.9.2