TM and interp disagree on function equality for null-closure

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jandem, Assigned: dvander)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][fixed-in-tracemonkey])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 503258 [details]
Test case

The attached test case prints 'PASS' with interpreter/-m, but prints 'FAIL' with -j. It looks like TM does not create a new function object for a function expression?
blocking2.0: --- → ?

Updated

8 years ago
blocking2.0: ? → betaN+
Whiteboard: hardblocker
Assignee: general → dmandelin
Assignee: dmandelin → dvander
Here the interpreter and tracer are taking different paths in JSOP_LAMBDA. The lambda is a null closure. The interpreter decides to clone because (funobj->getParent() != fp->scopeChain()).

Meanwhile, the tracer decides that it should *not* clone because (funobj->getParent() == globalObj).

Comment 2

8 years ago
Is one behavior or the other "correct"?
Summary: TM: different output for test case involving function equality → TM and interp disagree on function equality for null-closure
Yes.  The correct behavior is to print "PASS".  Per ES5 spec, each function expression creates a new object.  Under the hood we actually don't do that in some cases as an optimization, and in this case that's leaking out into script.

dvander, any idea why TM is doing the test it does?
(In reply to comment #1)
> Here the interpreter and tracer are taking different paths in JSOP_LAMBDA. The
> lambda is a null closure. The interpreter decides to clone because
> (funobj->getParent() != fp->scopeChain()).
> 
> Meanwhile, the tracer decides that it should *not* clone because
> (funobj->getParent() == globalObj).

Old bug. The tracer used to be able to assume that only the global object was on the scope chain.

/be
Created attachment 505383 [details] [diff] [review]
fix?

It looks like the guard in question came in with bug 495331. I have no idea why it introduced that test, but it makes sense given that the compiler can give null closures weird scope chains.

This patch just makes the tracer match the interpreter, by effectively only avoiding cloning if both fp->scopeChain() and fun->parent are globalObj.
Attachment #505383 - Flags: review?(brendan)
Comment on attachment 505383 [details] [diff] [review]
fix?

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -15426,31 +15426,31 @@ TraceRecorder::getFullIndex(ptrdiff_t pc
> }
> 
> JS_REQUIRES_STACK AbortableRecordingStatus
> TraceRecorder::record_JSOP_LAMBDA()
> {
>     JSFunction* fun;
>     fun = cx->fp()->script()->getFunction(getFullIndex());
> 
>+    if (FUN_CLOSURE(fun) && FUN_OBJECT(fun)->getParent() != globalObj)
>+        RETURN_STOP_A("Null closure function object parent must be global object");

FUN_NULL_CLOSURE(fun), right? Need test too.

/be
Created attachment 506628 [details] [diff] [review]
fix

Sorry about that mess.
Attachment #505383 - Attachment is obsolete: true
Attachment #506628 - Flags: review?(brendan)
Attachment #505383 - Flags: review?(brendan)
Comment on attachment 506628 [details] [diff] [review]
fix

I'm jazzed to have a test, hate to quibble, but consistently crunched var i=0; in for loops, or relaxed style with spaces around < too, would be just that much prettier. Thanks,

/be
Attachment #506628 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/6962ba9824d7 with nits
blocking2.0: betaN+ → ---
Whiteboard: hardblocker → [hardblocker][fixed-in-tracemonkey]
Status: NEW → ASSIGNED
blocking2.0: --- → betaN+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.