TM: Incorrect output with traced JSOP_BINDNAME

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

({verified1.9.2})

Trunk
verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.

Updated

9 years ago
Flags: blocking1.9.2+
(Assignee)

Comment 1

9 years ago
Created attachment 391248 [details] [diff] [review]
Patch
Attachment #391248 - Flags: review?(gal)
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
(Assignee)

Comment 3

9 years ago
(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.
Assert harder to cover the assumptions with some DEBUG-built checking?

/be
(Assignee)

Comment 5

9 years ago
Created attachment 391490 [details] [diff] [review]
Patch 2

Added an assert.
Attachment #391248 - Attachment is obsolete: true
Attachment #391490 - Flags: review?(gal)
Attachment #391248 - Flags: review?(gal)

Updated

9 years ago
Attachment #391490 - Flags: review?(gal) → review+

Comment 6

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

Comment 7

9 years ago
Pushed to TM with extra newlines (agreed it looks better) as be4280409fb3.
Depends on: 507657
(Assignee)

Updated

9 years ago
Duplicate of this bug: 507678

Comment 9

9 years ago
http://hg.mozilla.org/mozilla-central/rev/be4280409fb3
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 510437
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)
Keywords: fixed1.9.2
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
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
Keywords: verified1.9.2

Comment 12

9 years ago
js/tests/js1_8_1/regress/regress-507053.js
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.