625157 - TM and interp disagree on function equality for null-closure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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?

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[Jesse Ruderman]

Is one behavior or the other "correct"?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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.

Comment 6

[Brendan Eich [: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

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Sorry about that mess.

Comment 8

[Brendan Eich [: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

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/6962ba9824d7 with nits

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/6962ba9824d7