harmony_scoping: Implement lexical bindings at top level

harmony_scoping: Implement lexical bindings at top level

This implements correct semantics for "extensible" top level lexical scope.
The entire lexical scope is represented at runtime by GlobalContextTable, reachable from native context and accumulating global contexts from every script loaded into the context.

When the new script starts executing, it does the following validation:
- checks the GlobalContextTable and global object (non-configurable own) properties against the set of declarations it introduces and reports potential conflicts.
- invalidates the conflicting PropertyCells on global object, so that any code depending on them will miss/deopt causing any contextual lookups to be reexecuted under the new bindings
- adds the lexical bindings it introduces to the GlobalContextTable

Loads and stores for contextual lookups are modified so that they check the GlobalContextTable before looking up properties on global object, thus implementing the shadowing of global object properties by lexical declarations.

R=adamk@chromium.org, rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=25220

[Dmitry Lomov (no reviews), 2014-11-05 16:54:33]

FYI: this is work in progress, but shaping up. Let me know if you have any
comments.

[rossberg, 2014-11-06 12:29:12]

https://codereview.chromium.org/705663004/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/1/src/contexts.cc#newcode24
src/contexts.cc:24: FixedArray::CopySize(table, length + 1));
This should perhaps double the table size, pre-allocating some slots.

https://codereview.chromium.org/705663004/diff/1/src/contexts.cc#newcode147
src/contexts.cc:147: // Note: Fixed context slots are statically allocated by
the compiler.
Nit: why not move this comment next to the UNREACHABLE below?

https://codereview.chromium.org/705663004/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/705663004/diff/1/src/contexts.h#newcode199
src/contexts.h:199: int context_index_;
Nit: public data fields don't have a trailing "_".

https://codereview.chromium.org/705663004/diff/1/src/contexts.h#newcode203
src/contexts.h:203: MaybeAssignedFlag maybe_assigned_flag_;
Do we actually need the two flags for the global context table?

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (left):

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:522:
result->global_object()->set_global_context(*result);
Can't the global_context property on the global object be removed altogether
now?

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:518: if (IsLexicalVariableMode(mode) ||
IsLexicalVariableMode(lookup.mode_)) {
Are non-lexical variables actually included in those?

As discussed off-line, there needs to be an additional check against
non-configurable properties on the global object.

In fact, I think you also need to implement the inverse check in DeclareGlobals,
to diagnose 'var' clashing with earlier 'let'.

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:540: if (FindNameClash(scope_info,
global_context_table, &clashed_name)) {
Now that the checking this is a runtime thing and we don't have the global scope
chain anymore, I wonder if there aren't various places where special handling of
global scope or global context objects can be removed.

[Dmitry Lomov (no reviews), 2014-11-06 17:26:58]

This is now ready for review. I believe this is semantically correct and does
not degrade performance when no let declarations are present.

Subsequent CLs: 
- LoadIC and StoreIC handlers for global context loads.
- Cleanup of global context/global scope processing
- Cleanup of compilation cache

https://codereview.chromium.org/705663004/diff/1/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/1/src/contexts.cc#newcode24
src/contexts.cc:24: FixedArray::CopySize(table, length + 1));
On 2014/11/06 12:29:12, rossberg wrote:
> This should perhaps double the table size, pre-allocating some slots.

Done.

https://codereview.chromium.org/705663004/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/705663004/diff/1/src/contexts.h#newcode199
src/contexts.h:199: int context_index_;
On 2014/11/06 12:29:12, rossberg wrote:
> Nit: public data fields don't have a trailing "_".

Done.

https://codereview.chromium.org/705663004/diff/1/src/contexts.h#newcode203
src/contexts.h:203: MaybeAssignedFlag maybe_assigned_flag_;
On 2014/11/06 12:29:12, rossberg wrote:
> Do we actually need the two flags for the global context table?

They are used in Context::Lookup.

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (left):

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:522:
result->global_object()->set_global_context(*result);
On 2014/11/06 12:29:12, rossberg wrote:
> Can't the global_context property on the global object be removed altogether
> now?

Yes, I will do the cleanup in separate CL.

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:518: if (IsLexicalVariableMode(mode) ||
IsLexicalVariableMode(lookup.mode_)) {
On 2014/11/06 12:29:12, rossberg wrote:
> Are non-lexical variables actually included in those?
> 
> As discussed off-line, there needs to be an additional check against
> non-configurable properties on the global object.
> 
> In fact, I think you also need to implement the inverse check in
DeclareGlobals,
> to diagnose 'var' clashing with earlier 'let'.
All done (I kept IsLexicalVariableMode(lookup.mode) check for conistency

https://codereview.chromium.org/705663004/diff/1/src/runtime/runtime-scopes.c...
src/runtime/runtime-scopes.cc:540: if (FindNameClash(scope_info,
global_context_table, &clashed_name)) {
On 2014/11/06 12:29:12, rossberg wrote:
> Now that the checking this is a runtime thing and we don't have the global
scope
> chain anymore, I wonder if there aren't various places where special handling
of
> global scope or global context objects can be removed.

Yes, I will clean it up in separate CL

[adamk, 2014-11-06 20:18:06]

Sorry for the delay (due to BlinkOn).

Would you mind adding a more detailed CL description?

Mostly-stylistic comments below.

https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:912: factory->NewGlobalContextTable(1, 0);
This pair of numbers is a bit hard to read, and as far as I can tell this is the
only caller. Can we just leave out the arguments for now?

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode20
src/contexts.cc:20: CHECK(size >= 0 && length > 0 && size < length);
This looks like the sort of check I'd expect in objects-debug.cc. Perhaps
GlobalContextTable wants a verifier?

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode22
src/contexts.cc:22: CHECK(length < Smi::kMaxValue / 2);
This is basically a crash-if-OOM sort of check, right? Seems a little scary to
crash right here, but maybe there's no alternative...

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode190
src/contexts.h:190: class GlobalContextTable : public FixedArray {
Please add a comment for this class. What is it for? What's the layout?

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode206
src/contexts.h:206: int size() const { return
Smi::cast(get(kSizeSlot))->value(); }
Just a question: are const methods that common? I do see a few in objects.h now
that I look. What's the best practice here?

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode210
src/contexts.h:210: return Handle<Context>::cast(FixedArray::get(table, i + 1));
This and the various other +1s in the .cc file might benefit from a constant or
a private helper method, but I could imagine that's too much overhead for a
simple case like this. I leave it up to you.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode213
src/contexts.h:213: static bool Lookup(Handle<GlobalContextTable> table,
Handle<String> name,
This method could use a comment, at least for the return value. It's notable
that result ends up with garbage in it when this returns false (MUST_USE_RESULT
wouldn't be a bad idea here for that reason).

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode216
src/contexts.h:216: // table may be null.
Not clear what this means, it certainly can't be a null handle.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode217
src/contexts.h:217: static Handle<GlobalContextTable>
Extend(Handle<GlobalContextTable> table,
MUST_USE_RESULT?

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode222
src/contexts.h:222: };
nit: DISALLOW_IMPLICIT_CONSTRUCTORS

https://codereview.chromium.org/705663004/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/705663004/diff/20001/src/factory.h#newcode233
src/factory.h:233: Handle<GlobalContextTable> NewGlobalContextTable(int length,
int size);
As noted elsewhere, seems like you could just leave these arguments off until
there's a caller that needs them (especially since you already have the comment
above).

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:518: static Object*
FindNameClash(Handle<ScopeInfo> scope_info,
Not necessary in this patch, but this file could use some MaybeHandlification.

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:536: if (!maybe.has_value) return
isolate->heap()->exception();
Why return exception here?

[Dmitry Lomov (no reviews), 2014-11-07 10:18:48]

Comments addressed, PTAL

https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/bootstrapper.cc#newc...
src/bootstrapper.cc:912: factory->NewGlobalContextTable(1, 0);
On 2014/11/06 20:18:05, adamk wrote:
> This pair of numbers is a bit hard to read, and as far as I can tell this is
the
> only caller. Can we just leave out the arguments for now?

Done, renamed to NewEmptyGlobalContextTable().

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode20
src/contexts.cc:20: CHECK(size >= 0 && length > 0 && size < length);
On 2014/11/06 20:18:05, adamk wrote:
> This looks like the sort of check I'd expect in objects-debug.cc. Perhaps
> GlobalContextTable wants a verifier?

This check is here for int-overflow protection. 
Verifier dispatch in objects-debug.cc is on instance type, and
GlobalContextTable does not add an instance type, so specific verifier for it is
more trouble that its worth, I think.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.cc#newcode22
src/contexts.cc:22: CHECK(length < Smi::kMaxValue / 2);
On 2014/11/06 20:18:05, adamk wrote:
> This is basically a crash-if-OOM sort of check, right? Seems a little scary to
> crash right here, but maybe there's no alternative...

No this is a sanity/correctness check to make sure that length * 2 does not
overflow MaxSmi and does not int-overflow. 2^29 is a lot of top-level scripts,
so I guess we will go out of memory way before this hits.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode190
src/contexts.h:190: class GlobalContextTable : public FixedArray {
On 2014/11/06 20:18:06, adamk wrote:
> Please add a comment for this class. What is it for? What's the layout?

Done.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode206
src/contexts.h:206: int size() const { return
Smi::cast(get(kSizeSlot))->value(); }
On 2014/11/06 20:18:05, adamk wrote:
> Just a question: are const methods that common? I do see a few in objects.h
now
> that I look. What's the best practice here?

Make methods 'const' if possible is the parctice

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode210
src/contexts.h:210: return Handle<Context>::cast(FixedArray::get(table, i + 1));
On 2014/11/06 20:18:06, adamk wrote:
> This and the various other +1s in the .cc file might benefit from a constant
or
> a private helper method, but I could imagine that's too much overhead for a
> simple case like this. I leave it up to you.

Yes, I think it is too much fuss.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode213
src/contexts.h:213: static bool Lookup(Handle<GlobalContextTable> table,
Handle<String> name,
On 2014/11/06 20:18:06, adamk wrote:
> This method could use a comment, at least for the return value. It's notable
> that result ends up with garbage in it when this returns false
(MUST_USE_RESULT
> wouldn't be a bad idea here for that reason).

Done.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode216
src/contexts.h:216: // table may be null.
On 2014/11/06 20:18:05, adamk wrote:
> Not clear what this means, it certainly can't be a null handle.

Stray comment, removed.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode217
src/contexts.h:217: static Handle<GlobalContextTable>
Extend(Handle<GlobalContextTable> table,
On 2014/11/06 20:18:05, adamk wrote:
> MUST_USE_RESULT?

Done.

https://codereview.chromium.org/705663004/diff/20001/src/contexts.h#newcode222
src/contexts.h:222: };
On 2014/11/06 20:18:05, adamk wrote:
> nit: DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

https://codereview.chromium.org/705663004/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/705663004/diff/20001/src/factory.h#newcode233
src/factory.h:233: Handle<GlobalContextTable> NewGlobalContextTable(int length,
int size);
On 2014/11/06 20:18:06, adamk wrote:
> As noted elsewhere, seems like you could just leave these arguments off until
> there's a caller that needs them (especially since you already have the
comment
> above).

Done.

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:518: static Object*
FindNameClash(Handle<ScopeInfo> scope_info,
On 2014/11/06 20:18:06, adamk wrote:
> Not necessary in this patch, but this file could use some MaybeHandlification.

Indeed. I'll prep a clean-up patch.

https://codereview.chromium.org/705663004/diff/20001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:536: if (!maybe.has_value) return
isolate->heap()->exception();
On 2014/11/06 20:18:06, adamk wrote:
> Why return exception here?

Absence of value in Maybe<T> indicates exception, so we propagate that.
When GetPropertyAttributes cannot find a property, it returns maybe(ABSENT), so
the code below will execute even for absent properties, but it is no harm done.

[rossberg, 2014-11-07 13:22:38]

Nice, only nits remaining.

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode18
src/contexts.cc:18: int size = table->size();
Nit: maybe call this 'used' here for more clarity.

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode20
src/contexts.cc:20: CHECK(size >= 0 && length > 0 && size < length);
Nit: The second condition is already implied by the other two.

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode28
src/contexts.cc:28: result->set(kSizeSlot, Smi::FromInt(size + 1));
Introduce a set_size accessor for this.

https://codereview.chromium.org/705663004/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/705663004/diff/40001/src/factory.h#newcode233
src/factory.h:233: Handle<GlobalContextTable> NewEmptyGlobalContextTable();
Nit: Drop the 'Empty', unless there is another version as well.

[Dmitry Lomov (no reviews), 2014-11-07 14:29:00]

All nits addressed, PTAL

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode18
src/contexts.cc:18: int size = table->size();
On 2014/11/07 13:22:38, rossberg wrote:
> Nit: maybe call this 'used' here for more clarity.

Done.

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode20
src/contexts.cc:20: CHECK(size >= 0 && length > 0 && size < length);
On 2014/11/07 13:22:38, rossberg wrote:
> Nit: The second condition is already implied by the other two.

Will keep as is for clarity.

https://codereview.chromium.org/705663004/diff/40001/src/contexts.cc#newcode28
src/contexts.cc:28: result->set(kSizeSlot, Smi::FromInt(size + 1));
On 2014/11/07 13:22:38, rossberg wrote:
> Introduce a set_size accessor for this.

Done.

https://codereview.chromium.org/705663004/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/705663004/diff/40001/src/factory.h#newcode233
src/factory.h:233: Handle<GlobalContextTable> NewEmptyGlobalContextTable();
On 2014/11/07 13:22:38, rossberg wrote:
> Nit: Drop the 'Empty', unless there is another version as well.

Done.

[rossberg, 2014-11-07 14:36:10]

lgtm

https://codereview.chromium.org/705663004/diff/80001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/705663004/diff/80001/src/contexts.h#newcode193
src/contexts.h:193: // The table is a fixed array, its first slot is current
used cound and
Typo: s/cound/count/

Also, "is [the] current"

[Dmitry Lomov (no reviews), 2014-11-07 14:41:59]

Thanks for review! 
Adam, could you take a look as well?
On 2014/11/07 14:36:10, rossberg wrote:
> lgtm
> 
> https://codereview.chromium.org/705663004/diff/80001/src/contexts.h
> File src/contexts.h (right):
> 
> https://codereview.chromium.org/705663004/diff/80001/src/contexts.h#newcode193
> src/contexts.h:193: // The table is a fixed array, its first slot is current
> used cound and
> Typo: s/cound/count/
> 
> Also, "is [the] current"
Ah, thanks!

[adamk, 2014-11-07 16:04:03]

lgtm

[Dmitry Lomov (no reviews), 2014-11-07 16:29:26]

Committed patchset #6 (id:100001) manually as 25220 (presubmit successful).