Bug 18634 - SQUIRRELFISH: correctly handle variable and function declarations in eval code
Summary: SQUIRRELFISH: correctly handle variable and function declarations in eval code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 18624
  Show dependency treegraph
 
Reported: 2008-04-19 23:16 PDT by Cameron Zwarich (cpst)
Modified: 2008-04-22 20:17 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch for variable declarations (9.69 KB, patch)
2008-04-20 22:43 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff
Revised proposed patch (9.65 KB, patch)
2008-04-20 23:27 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff
Revised proposed patch (9.64 KB, patch)
2008-04-21 00:32 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff
Proposed patch implementing function declarations in eval code (4.28 KB, patch)
2008-04-22 19:16 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (4.91 KB, patch)
2008-04-22 19:59 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-04-19 23:16:11 PDT
SquirrelFish currently doesn't correctly handle variable and function declarations in eval code. The problems are as follows:

1) Variables that are declared in eval code are not initialized to undefined at the beginning of the eval code block. This affects at least the following tests, in the simple form of a "var MYVAR;" declaration:

ecma/Expressions/11.3.1.js
ecma/Expressions/11.3.2.js
ecma/Expressions/11.4.4.js
ecma/Expressions/11.4.5.js

2) Variables defined in eval code are always initialized in global scope. This is a side effect of the above problem. If the first problem was fixed and undefined was stored in the slot of each new variable, then the code emitted for AssignResolveNode would find the right base for the variable and update that object correctly. We need to be careful here that we always use the right variable object, which is the activation object of the calling function in the case of direct eval inside of a function body, and the global object otherwise. We can't just grab the top of the scope chain thanks to 'with', and we no longer correctly track variable objects in ExecStates, but we should still be able to determine whether or not we are global code and grab the activation object of the calling function via callFrame[Machine::OptionalCalleeActivation] if we are not.

3) Function declarations in eval code don't work at all. I think this will be easily fixable once we fix the other two problems.
Comment 1 Cameron Zwarich (cpst) 2008-04-20 22:43:14 PDT
Created attachment 20714 [details]
Proposed patch for variable declarations

Here is a patch that adds support for variable declarations but not function declarations.
Comment 2 Maciej Stachowiak 2008-04-20 23:17:31 PDT
Comment on attachment 20714 [details]
Proposed patch for variable declarations

r=me
Comment 3 Cameron Zwarich (cpst) 2008-04-20 23:27:09 PDT
Created attachment 20716 [details]
Revised proposed patch

Made a minor change as per Maciej's suggestion on IRC. Strangely enough, this is a 1.5% performance progression on SunSpider.
Comment 4 Maciej Stachowiak 2008-04-20 23:28:36 PDT
Comment on attachment 20716 [details]
Revised proposed patch

r=me
Comment 5 Cameron Zwarich (cpst) 2008-04-21 00:32:12 PDT
Created attachment 20718 [details]
Revised proposed patch

It turns out that the cause of the speedup was a GC bug in string-tagcloud, where the EvalCodeBlocks were not getting marked. Here is a patch that fixes the GC bug by treating EvalCodeBlocks like ProgramCodeBlocks. This time there is no difference on SunSpider.
Comment 6 Maciej Stachowiak 2008-04-21 00:33:15 PDT
Comment on attachment 20718 [details]
Revised proposed patch

r=me
Comment 7 Cameron Zwarich (cpst) 2008-04-21 22:19:32 PDT
This bug isn't quite fixed yet. The patch to support variable declarations landed in r32290, but there are still function declarations to support. I'll hopefully upload a patch soon.
Comment 8 Cameron Zwarich (cpst) 2008-04-22 19:16:42 PDT
Created attachment 20761 [details]
Proposed patch implementing function declarations in eval code

This patch implements function declarations in eval code. In the process, it fixes 12 JavaScriptCore test failures.

Should declaredVariables and declaredFunctions be declaredVariableIdentifiers and declaredFunctionIdentifiers?
Comment 9 Cameron Zwarich (cpst) 2008-04-22 19:59:43 PDT
Created attachment 20762 [details]
Revised proposed patch

Here is the same patch with a slight change in naming to make things more clear. SunSpider reports a 0.5% performance progression from ToT, which is mostly concentrated on the math tests.
Comment 10 Maciej Stachowiak 2008-04-22 20:07:24 PDT
Comment on attachment 20762 [details]
Revised proposed patch

r=me