[modules] Partial scope info support of modules

[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
Committed: https://crrev.com/840d1e84f74ebc5a596aad20b23b29025c61cf6c
Cr-Commit-Position: refs/heads/master@{#39049}

[neis, 2016-08-25 14:47:45]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:47:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:18:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:18:26]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-08-25 16:10:13]

Description was changed from

==========
[modules] Partial support of modules in scopeinfos.
R=adamk@chromium.org
BUG=
==========

to

==========
[modules] Partial support of modules in scopeinfos.

R=adamk@chromium.org
BUG=v8:1569
==========

[adamk, 2016-08-25 16:19:43]

Looks like this is in something of an early state? As it comes together more
please flesh out that CL description.

https://codereview.chromium.org/2277253003/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2277253003/diff/1/src/api.cc#newcode23
src/api.cc:23: #include "src/ast/scopeinfo.h"
Is this needed yet?

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h
File src/ast/scopeinfo.h (right):

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h#newcode15
src/ast/scopeinfo.h:15: class ModuleInfo : public FixedArray {
I think there's some desire to keep all HeapObjects in objects.h (and to keep
the comment at the top of objects.h updated).

That also means you'd want to add heap verification support and maybe printing
support.

This class should get a comment explaining its layout too (probably similar to
the one that's in the serialization code right now).

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc#newcode456
src/ast/scopes.cc:456: if (scope->is_module_scope())
Please add braces if this  doesn't fit on one line.

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc#newcode868
src/ast/scopes.cc:868: if (var->IsStackLocal()) {  // XXX use switch
Remove this comment before landing (or switch to a switch before landing?)

https://codereview.chromium.org/2277253003/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/2277253003/diff/1/src/handles.h#newcode102
src/handles.h:102: V8_INLINE Handle(S* object, Isolate* isolate)
What is this for?

[neis, 2016-08-26 09:52:07]

https://codereview.chromium.org/2277253003/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2277253003/diff/1/src/api.cc#newcode23
src/api.cc:23: #include "src/ast/scopeinfo.h"
On 2016/08/25 16:19:43, adamk wrote:
> Is this needed yet?

Not in this CL, no.  Leftover of trying to split my changes into digestible CLs.

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc#newcode456
src/ast/scopes.cc:456: if (scope->is_module_scope())
On 2016/08/25 16:19:43, adamk wrote:
> Please add braces if this  doesn't fit on one line.

Done.

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopes.cc#newcode868
src/ast/scopes.cc:868: if (var->IsStackLocal()) {  // XXX use switch
On 2016/08/25 16:19:43, adamk wrote:
> Remove this comment before landing (or switch to a switch before landing?)

Turned it into a switch (above as well).

https://codereview.chromium.org/2277253003/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/2277253003/diff/1/src/handles.h#newcode102
src/handles.h:102: V8_INLINE Handle(S* object, Isolate* isolate)
On 2016/08/25 16:19:43, adamk wrote:
> What is this for?

I added it to save some typing but I removed it now.

[neis, 2016-08-26 10:06:20]

Description was changed from

==========
[modules] Partial support of modules in scopeinfos.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Partial support of modules in scopeinfos.

This introduces a new heap object ModuleInfo, which is for ModuleDescriptor
what ScopeInfo is for Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

[neis, 2016-08-26 12:10:26]

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h
File src/ast/scopeinfo.h (right):

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h#newcode15
src/ast/scopeinfo.h:15: class ModuleInfo : public FixedArray {
On 2016/08/25 16:19:43, adamk wrote:
> I think there's some desire to keep all HeapObjects in objects.h (and to keep
> the comment at the top of objects.h updated).
> 
> That also means you'd want to add heap verification support and maybe printing
> support.
> 
> This class should get a comment explaining its layout too (probably similar to
> the one that's in the serialization code right now).

I moved ModuleInfo into objects.h and removed scopeinfo.h again.  I'm keeping it
simple for now by not doing providing its own printing and verification
functions. But as ModuleInfo becomes more complex I will add these.

[adamk, 2016-08-26 16:54:44]

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h
File src/ast/scopeinfo.h (right):

https://codereview.chromium.org/2277253003/diff/1/src/ast/scopeinfo.h#newcode15
src/ast/scopeinfo.h:15: class ModuleInfo : public FixedArray {
On 2016/08/26 12:10:26, neis wrote:
> On 2016/08/25 16:19:43, adamk wrote:
> > I think there's some desire to keep all HeapObjects in objects.h (and to
keep
> > the comment at the top of objects.h updated).
> > 
> > That also means you'd want to add heap verification support and maybe
printing
> > support.
> > 
> > This class should get a comment explaining its layout too (probably similar
to
> > the one that's in the serialization code right now).
> 
> I moved ModuleInfo into objects.h and removed scopeinfo.h again.  I'm keeping
it
> simple for now by not doing providing its own printing and verification
> functions. But as ModuleInfo becomes more complex I will add these.

Can you at least provide a short comment on ModuleInfo?

https://codereview.chromium.org/2277253003/diff/60001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2277253003/diff/60001/src/ast/scopeinfo.cc#ne...
src/ast/scopeinfo.cc:505: int ScopeInfo::ModuleIndex(Handle<ScopeInfo>
scope_info, Handle<String> name,
This function is full of magic constants. Is there some way to add either more
helper methods or constants to clean it up?

https://codereview.chromium.org/2277253003/diff/60001/src/ast/scopeinfo.cc#ne...
src/ast/scopeinfo.cc:759: int i = 0;
Not anything wrong with your CL, but it makes me sad that C++11 doesn't have a
nicer way to mix range-based and indexed for loops.

https://codereview.chromium.org/2277253003/diff/60001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2277253003/diff/60001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:863: for (int i = 0; i < temps->length(); i++) {
I think this loop is gone if you merge to ToT.

https://codereview.chromium.org/2277253003/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2277253003/diff/60001/src/factory.cc#newcode1397
src/factory.cc:1397: Handle<FixedArray> array = NewFixedArray(2, TENURED);
More magic constants...I think this one should be a constant on ModuleInfo.

[neis, 2016-08-29 14:04:38]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 14:04:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 14:05:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 14:05:55]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11770)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7838)

[neis, 2016-08-29 15:26:27]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 15:26:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[neis, 2016-08-29 15:27:32]

Description was changed from

==========
[modules] Partial support of modules in scopeinfos.

This introduces a new heap object ModuleInfo, which is for ModuleDescriptor
what ScopeInfo is for Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Partial support of modules in scopeinfos.

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

[neis, 2016-08-29 15:29:08]

Please have another look.  I've tried to address your comments and I had to do a
significant change after rebasing on Toon's CL that got rid of CollectVariables
(former CollectLocalContextSlotsAndStackSlotsAndMaybeSomeOtherStuff).

https://codereview.chromium.org/2277253003/diff/140001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2277253003/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:239: index += kModuleVariableEntryLength *
module_vars_count;
This artificial incrementing of index here and above (after the switch) is a bit
weird.  I guess that originally the idea was to always write to index++ in order
to make it obvious that there are no unused slots?  But it's already not obvious
anymore and my change makes it even less clear.

[commit-bot: I haz the power, 2016-08-29 15:50:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 15:50:26]

Dry run: This issue passed the CQ dry run.

[neis, 2016-08-30 09:56:40]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 09:56:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 09:58:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 09:58:55]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7879)

[neis, 2016-08-30 11:08:06]

Description was changed from

==========
[modules] Partial support of modules in scopeinfos.

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

[neis, 2016-08-30 11:08:21]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 11:08:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 11:34:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 11:34:55]

Dry run: This issue passed the CQ dry run.

[neis, 2016-08-30 15:55:07]

ready now

[Toon Verwaest, 2016-08-30 17:23:43]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-08-30 17:23:43]

Just for the record, I don't think there's a desire to bundle up stuff in
objects.h. we're only cautious about moving things out of other object files
that rely on the macros defined there. I'm all in favor of putting classes in
better places. ScopeInfo living in objects.h is confusing, especially with the
implementation living in ScopeInfo.cc

[adamk, 2016-08-30 20:21:41]

This lgtm now

I'm happy to defer to Toon's experience if we think this should live in
scopeinfo.h. Or we could move it over there along with ScopeInfo itself in a
separate patch.

[neis, 2016-08-31 07:45:06]

neis@chromium.org changed reviewers:
+ ulan@chromium.org

[neis, 2016-08-31 07:45:06]

+ulan for heap, thanks!

[neis, 2016-08-31 07:45:09]

+ulan for heap, thanks!

[ulan, 2016-08-31 09:30:39]

heap lgtm

[neis, 2016-08-31 12:53:06]

The CQ bit was checked by neis@chromium.org

[neis, 2016-08-31 12:53:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2277253003/#ps220001
(title: "Another rebase.")

[commit-bot: I haz the power, 2016-08-31 12:53:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 13:34:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 13:34:46]

Failed to apply patch for src/ast/scopes.cc:
While running git apply --index -3 -p1;
  error: patch failed: src/ast/scopes.cc:427
  Falling back to three-way merge...
  Applied patch to 'src/ast/scopes.cc' with conflicts.
  U src/ast/scopes.cc

Patch:       src/ast/scopes.cc
Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index
f073b0e7a53874ef497033187189670c9207e293..84db6bf75d22ea8cba03dc86f18d2fdb97788da4
100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -157,6 +157,36 @@ ModuleScope::ModuleScope(DeclarationScope* script_scope,
   DeclareThis(ast_value_factory);
 }
 
+ModuleScope::ModuleScope(Isolate* isolate, Handle<ScopeInfo> scope_info,
+                         AstValueFactory* avfactory)
+    : DeclarationScope(avfactory->zone(), MODULE_SCOPE, scope_info) {
+  Zone* zone = avfactory->zone();
+  ModuleInfo* module_info = scope_info->ModuleDescriptorInfo();
+
+  set_language_mode(STRICT);
+  module_descriptor_ = new (zone) ModuleDescriptor(zone);
+
+  // Deserialize special exports.
+  Handle<FixedArray> special_exports = handle(module_info->special_exports());
+  for (int i = 0, n = special_exports->length(); i < n; ++i) {
+    Handle<FixedArray> serialized_entry(
+        FixedArray::cast(special_exports->get(i)), isolate);
+    module_descriptor_->AddSpecialExport(
+        ModuleDescriptor::Entry::Deserialize(isolate, avfactory,
+                                             serialized_entry),
+        avfactory->zone());
+  }
+
+  // Deserialize regular exports.
+  Handle<FixedArray> regular_exports = handle(module_info->regular_exports());
+  for (int i = 0, n = regular_exports->length(); i < n; ++i) {
+    Handle<FixedArray> serialized_entry(
+        FixedArray::cast(regular_exports->get(i)), isolate);
+    module_descriptor_->AddRegularExport(ModuleDescriptor::Entry::Deserialize(
+        isolate, avfactory, serialized_entry));
+  }
+}
+
 Scope::Scope(Zone* zone, ScopeType scope_type, Handle<ScopeInfo> scope_info)
     : zone_(zone),
       outer_scope_(nullptr),
@@ -320,6 +350,11 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone*
zone,
       } else {
         outer_scope = new (zone) Scope(zone, BLOCK_SCOPE, scope_info);
       }
+    } else if (context->IsModuleContext()) {
+      ScopeInfo* scope_info = context->closure()->shared()->scope_info();
+      DCHECK_EQ(scope_info->scope_type(), MODULE_SCOPE);
+      outer_scope = new (zone) ModuleScope(
+          isolate, Handle<ScopeInfo>(scope_info), ast_value_factory);
     } else {
       DCHECK(context->IsCatchContext());
       String* name = context->catch_name();
@@ -427,6 +462,12 @@ void DeclarationScope::Analyze(ParseInfo* info, AnalyzeMode
mode) {
         info->ast_value_factory()->this_string(), Variable::THIS);
   }
 
+  // For modules, we want to start variable allocation at the surrounding
script
+  // scope.
+  if (scope->is_module_scope()) {
+    scope = scope->outer_scope()->AsDeclarationScope();
+  }
+
   scope->AllocateVariables(info, mode);
 
 #ifdef DEBUG
@@ -616,7 +657,8 @@ Variable* Scope::LookupInScopeInfo(const AstRawString* name)
{
                                           &init_flag, &maybe_assigned_flag);
   if (index < 0 && scope_type() == MODULE_SCOPE) {
     location = VariableLocation::MODULE;
-    index = -1;  // TODO(neis): Find module variables in scope info.
+    index = scope_info_->ModuleIndex(name_handle, &mode, &init_flag,
+                                     &maybe_assigned_flag);
   }
   if (index < 0) return nullptr;  // Nowhere found.

[neis, 2016-08-31 13:39:52]

The CQ bit was checked by neis@chromium.org

[neis, 2016-08-31 13:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2277253003/#ps240001
(title: "This is so much fun.")

[commit-bot: I haz the power, 2016-08-31 13:39:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 14:25:36]

Description was changed from

==========
[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-08-31 14:25:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-08-31 14:26:19]

Description was changed from

==========
[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569
==========

to

==========
[modules] Partial scope info support of modules

This introduces a new heap object ModuleInfo, which is to ModuleDescriptor
what ScopeInfo is to Scope.  When deserializing a scope info that contains
a module info, we deserialize the module info into a module descriptor and
put it into the synthesized module scope.

Currently, module infos only store exports.

R=adamk@chromium.org
BUG=v8:1569

Committed: https://crrev.com/840d1e84f74ebc5a596aad20b23b29025c61cf6c
Cr-Commit-Position: refs/heads/master@{#39049}
==========

[commit-bot: I haz the power, 2016-08-31 14:26:21]

Patchset 13 (id:??) landed as
https://crrev.com/840d1e84f74ebc5a596aad20b23b29025c61cf6c
Cr-Commit-Position: refs/heads/master@{#39049}