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

Description Saam Barati 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.
Comment 1 Saam Barati 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.
Comment 2 Saam Barati 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?
Comment 3 Saam Barati 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.
Comment 4 Saam Barati 2015-05-04 15:30:55 PDT
Created attachment 252339 [details]
patch
Comment 5 Geoffrey Garen 2015-05-04 19:39:00 PDT
Comment on attachment 252339 [details]
patch

r=me
Comment 6 Saam Barati 2015-05-04 20:27:27 PDT
Comment on attachment 252339 [details]
patch

I'm seeing some flakiness on exceptionFuzz still.

Investigating.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2015-05-04 20:28:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2015-05-04 20:31:55 PDT
Re-opened since this is blocked by bug 144620
Comment 10 Saam Barati 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.
Comment 11 Saam Barati 2015-05-07 18:06:14 PDT
Created attachment 252666 [details]
patch

for CQ
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2015-05-07 19:00:47 PDT
All reviewed patches have been landed.  Closing bug.