Bug 144178 – Global functions should be initialized as JSFunctions in byte code

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

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]

(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]

(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]

Created attachment 252339 [details]
patch

[Geoffrey Garen]

Comment on attachment 252339 [details]
patch

r=me

[Saam Barati]

Comment on attachment 252339 [details]
patch

I'm seeing some flakiness on exceptionFuzz still.

Investigating.

[WebKit Commit Bot]

Comment on attachment 252339 [details]
patch

Clearing flags on attachment: 252339

Committed r183789: <http://trac.webkit.org/changeset/183789>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144620

[Saam Barati]

(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]

Created attachment 252666 [details]
patch

for CQ

[WebKit Commit Bot]

Comment on attachment 252666 [details]
patch

Clearing flags on attachment: 252666

Committed r183972: <http://trac.webkit.org/changeset/183972>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.