Closed Bug 620838 Opened 14 years ago Closed 13 years ago

Incorrect optimization to global with local function inside control flow

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

Attached file 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.
Attached patch PatchSplinter Review
Attachment #499207 - Flags: review?(jwalden+bmo)
blocking2.0: --- → betaN+
Blocks: 620649
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.
Attachment #499207 - Flags: review?(jwalden+bmo) → review+
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?
Whiteboard: fixed-in-tracemonkey
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.  :-)
http://hg.mozilla.org/mozilla-central/rev/5f29abee4321
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: