620838 - Incorrect optimization to global with local function inside control flow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

Attachment: Shell test case

The attached test case fails, because we incorrectly optimize the call to |g| inside |q| to skip straight to the global object. Currently, we check for possibly-aliasing constructs in the current function (DEFFUN, WITH), and suppress that optimization if so via the TCF_FUN_MIGHT_ALIAS_LOCALS flag.

The bug is that we don't propagate that flag downward (i.e., to enclosed lexical scopes). If any outer scope to a given scope X can introduce aliases, then scope X cannot skip straight to the global object.

Patch forthcoming.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 2

[Jeff Walden [:Waldo]]

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

>diff --git a/js/src/jsemit.h b/js/src/jsemit.h

> /*
>+ * The function or a function that encloses it may define new local names
>+ * at run time through means other than calling eval.
>  */
> #define TCF_FUN_MIGHT_ALIAS_LOCALS  0x4000000

"runtime" seems the more common spelling in js-land to me.

Comment 3

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/5f29abee4321

My personal usage is to write "run time" when it would be replaceable with "compile time", and "runtime" for other issues. grep reveals you are right about established usage in SM, so I switched.

By the way, what do you like put in comment comments for 'r='? Should it be 'waldo', or 'jwalden', or something else?

Comment 4

[Jeff Walden [:Waldo]]

As long as it clearly identifies the reviewer, which both those do, I don't think it matters.  I've seen both used historically.  But when I'm pushing patches for others that I've reviewed, I use r=jwalden.

To the extent that I have a preference, which is extremely minimal and has never been enough that I've actually said it to anyone, I prefer r=jwalden because r=Waldo will ping me if I'm on IRC.  :-)

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/5f29abee4321