| Summary: | DFG misses arguments tear-off for function.arguments if 'arguments' is used | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||||||||||
| Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | barraclough, dglazkov, fpizlo, oliver, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Geoffrey Garen
2012-09-09 22:08:18 PDT
Created attachment 163039 [details]
Patch
Comment on attachment 163039 [details]
Patch
Uploading for discussion.
This patch eliminates aliasing of the local 'arguments' property, and makes function.arguments always tear off.
Another way to fix this bug, and keep our current behavior without regression, is to use watchpoints.
Here's what I know about the state of function.arguments: - It's non-standard: Mozilla invented and then deprecated it; ES4 never mentioned it; and ES5 explicitly poisons it in strict mode. - Compatibility matrix of arguments == function.arguments: Firefox: never IE: never, but function.arguments == function.arguments Chrome: if caller uses 'arguments' Safari: if caller uses 'arguments' Compatibility matrix of what function.arguments contains: Firefox: current snapshot Chrome: current snapshot Safari: current snapshot IE: original snapshot Based on this matrix, the best compatibility story by market share seems to be: - function.arguments doesn't alias 'arguments' (IE + Firefox) - function.arguments snapshots current argument values (Chrome + Firefox + Safari) Comment on attachment 163039 [details]
Patch
I think that your change implies that using 'arguments' should not cause the DFG to assume that arguments (i.e. the actual arguments, not the 'arguments' local) are captured. You should probably make that part of this change, as well - or at least file a separate bug indicating that we should kill off that code in the future.
Other than that, I have no objections to this change.
Comment on attachment 163039 [details] Patch Attachment 163039 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13795743 New failing tests: fast/js/dfg-tear-off-function-dot-arguments.html Notions of "captured arguments" that may need fixup:
CodeBlock
capturedVars bitvector
needs to match variableAccessData isCaptured flag
ByteCodeParser initialization
All isCaptured() queries other than VariableAccessData queries should just forward to the CodeBlock.
Created attachment 163198 [details]
compatibility test
Created attachment 163199 [details]
compatibility test
Created attachment 163203 [details]
compatibility test
Created attachment 163204 [details]
compatibility test
Created attachment 163205 [details]
compatibility test
Committed r128111: <http://trac.webkit.org/changeset/128111> |