Bug 144178

Summary: Global functions should be initialized as JSFunctions in byte code
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, mark.lam, mmirman, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 144265, 144620, 144753    
Bug Blocks: 142944    
Attachments:
Description Flags
work in progress
none
patch
none
patch none

Saam Barati
Reported 2015-04-24 17:49:08 PDT
Currently, ProgramExecutable::initializeGlobalProperties will initialize global 'var's as undefined, and global functions as JSFunctions. We should have this function initialize global functions to undefined and initialize those functions as JSFunctions inside the BytecodeGenerator. This will make implementing lexical scoping at the global scope not insane. Because lexical scopes are created in byte code, creating JSFunctions inside initializeGlobalProperties will not be created with the correct scope. For example: ``` let foo = 20; function f() { return foo; } ``` Function 'f' should be able to see 'foo', even though 'foo' doesn't live on the global object. Function 'f' should still live on the global object.
Attachments
work in progress (8.58 KB, patch)
2015-04-25 16:19 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (11.48 KB, patch)
2015-05-04 15:30 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (11.48 KB, patch)
2015-05-07 18:06 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2015-04-25 16:19:13 PDT
Created attachment 251651 [details] work in progress All but two tests are passing. There are the failing tests: 1. A profiler test is failing (I haven't investigated). 2. An API test is failing; specifically: globalStaticFunction() test. 'globalStaticFunction' is resolving to undefined and not the globalStaticFunction defined inside apitests.js. I've spent some a good amount of time investigating this and every idea I have about what is wrong seems to be incorrect. If anyone has an idea about why this may be failing, I'd appreciate the help.
Saam Barati
Comment 2 2015-04-27 01:29:51 PDT
(In reply to comment #1) > Created attachment 251651 [details] > 2. An API test is failing; specifically: globalStaticFunction() test. > 'globalStaticFunction' is resolving to undefined and not the > globalStaticFunction defined inside apitests.js. > I've spent some a good amount of time investigating this and every idea I > have about what is wrong seems to be incorrect. > If anyone has an idea about why this may be failing, I'd appreciate the help. Okay, after some digging I think I now know what's wrong. JSScope::abstractAccess won't respect the GlobalObject's Structure's propertyAccessesAreCacheable() result if the identifier being resolved is in the GlobalObject's symbol table. This seems wrong to me. I think propertyAccessesAreCacheable() should be checked before checking the GlobalObject's symbol table. Can anyone confirm?
Saam Barati
Comment 3 2015-05-04 15:12:36 PDT
(In reply to comment #2) > (In reply to comment #1) > > Created attachment 251651 [details] > > > 2. An API test is failing; specifically: globalStaticFunction() test. > > 'globalStaticFunction' is resolving to undefined and not the > > globalStaticFunction defined inside apitests.js. > > I've spent some a good amount of time investigating this and every idea I > > have about what is wrong seems to be incorrect. > > If anyone has an idea about why this may be failing, I'd appreciate the help. > > Okay, after some digging I think I now know what's wrong. > JSScope::abstractAccess won't respect the GlobalObject's Structure's > propertyAccessesAreCacheable() result if the identifier being resolved is > in the GlobalObject's symbol table. This seems wrong to me. I think > propertyAccessesAreCacheable() should be checked before checking the > GlobalObject's symbol table. > > Can anyone confirm? This is wrong. SymbolTablePut/Get should have priority and is cacheable.
Saam Barati
Comment 4 2015-05-04 15:30:55 PDT
Created attachment 252339 [details] patch
Geoffrey Garen
Comment 5 2015-05-04 19:39:00 PDT
Comment on attachment 252339 [details] patch r=me
Saam Barati
Comment 6 2015-05-04 20:27:27 PDT
Comment on attachment 252339 [details] patch I'm seeing some flakiness on exceptionFuzz still. Investigating.
WebKit Commit Bot
Comment 7 2015-05-04 20:28:29 PDT
Comment on attachment 252339 [details] patch Clearing flags on attachment: 252339 Committed r183789: <http://trac.webkit.org/changeset/183789>
WebKit Commit Bot
Comment 8 2015-05-04 20:28:34 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 9 2015-05-04 20:31:55 PDT
Re-opened since this is blocked by bug 144620
Saam Barati
Comment 10 2015-05-07 01:03:37 PDT
(In reply to comment #6) > Comment on attachment 252339 [details] > patch > > I'm seeing some flakiness on exceptionFuzz still. > > Investigating. I've found out the reason: We now emit put_to_scope for these global functions. They will increment the exceptionFuzz count and if the target count is low enough, will cause the VM to throw when initializing the global functions. This thrown exceptionFuzz exception won't be caught because this code for emitting global functions is emitted before the code for the actual "program" and so the big try block won't have the proper range that includes this pre-"program" initialization. The idea of throwing an exception fuzz when initializing global functions doesn't make sense because if the count is reached, we will invariably get an uncaught exception. I'll see if there is a nice way to get around this.
Saam Barati
Comment 11 2015-05-07 18:06:14 PDT
Created attachment 252666 [details] patch for CQ
WebKit Commit Bot
Comment 12 2015-05-07 19:00:38 PDT
Comment on attachment 252666 [details] patch Clearing flags on attachment: 252666 Committed r183972: <http://trac.webkit.org/changeset/183972>
WebKit Commit Bot
Comment 13 2015-05-07 19:00:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.