[modules] Assign cell indices at validation time.

Instead of having a MODULE variable's index be 0 or 1, let it be the index of
its cell.  In this CL, we assign the indices but we continue to only use them to
distinguish imports from exports.  Actually using them to directly access the
cells will be done in a later CL.

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

Review-Url: https://codereview.chromium.org/2460233003
Cr-Commit-Position: refs/heads/master@{#40752}

src/ast/modules.cc

@@ -87,9 +87,8 @@ Handle<ModuleInfoEntry> ModuleDescriptor::Entry::Serialize(
   return ModuleInfoEntry::New(
       isolate, ToStringOrUndefined(isolate, export_name),
       ToStringOrUndefined(isolate, local_name),
-      ToStringOrUndefined(isolate, import_name),
-      Handle<Object>(Smi::FromInt(module_request), isolate), location.beg_pos,
-      location.end_pos);
+      ToStringOrUndefined(isolate, import_name), module_request, cell_index,
+      location.beg_pos, location.end_pos);
 }
 
 ModuleDescriptor::Entry* ModuleDescriptor::Entry::Deserialize(
@@ -102,7 +101,8 @@ ModuleDescriptor::Entry* ModuleDescriptor::Entry::Deserialize(
       isolate, avfactory, handle(entry->local_name(), isolate));
   result->import_name = FromStringOrUndefined(
       isolate, avfactory, handle(entry->import_name(), isolate));
-  result->module_request = Smi::cast(entry->module_request())->value();
+  result->module_request = entry->module_request();
+  result->cell_index = entry->cell_index();
   return result;
 }
 
@@ -113,19 +113,22 @@ Handle<FixedArray> ModuleDescriptor::SerializeRegularExports(Isolate* isolate,
   // names.  (Regular exports have neither import name nor module request.)
 
   ZoneVector<Handle<Object>> data(zone);
-  data.reserve(2 * regular_exports_.size());
+  data.reserve(3 * regular_exports_.size());
 
   for (auto it = regular_exports_.begin(); it != regular_exports_.end();) {
     // Find out how many export names this local name has.
     auto next = it;
     int size = 0;
     do {
+      DCHECK_EQ(it->second->local_name, next->second->local_name);
+      DCHECK_EQ(it->second->cell_index, next->second->cell_index);
       ++next;
       ++size;
     } while (next != regular_exports_.end() && next->first == it->first);
 
     Handle<FixedArray> export_names = isolate->factory()->NewFixedArray(size);
     data.push_back(it->second->local_name->string());
+    data.push_back(handle(Smi::FromInt(it->second->cell_index), isolate));
     data.push_back(export_names);
 
     // Collect the export names.
@@ -154,6 +157,7 @@ void ModuleDescriptor::DeserializeRegularExports(Isolate* isolate,
                                                  Handle<FixedArray> data) {
   for (int i = 0, length_i = data->length(); i < length_i;) {
     Handle<String> local_name(String::cast(data->get(i++)), isolate);
+    int cell_index = Smi::cast(data->get(i++))->value();
     Handle<FixedArray> export_names(FixedArray::cast(data->get(i++)), isolate);
 
     for (int j = 0, length_j = export_names->length(); j < length_j; ++j) {
@@ -163,6 +167,7 @@ void ModuleDescriptor::DeserializeRegularExports(Isolate* isolate,
           new (avfactory->zone()) Entry(Scanner::Location::invalid());
       entry->local_name = avfactory->GetString(local_name);
       entry->export_name = avfactory->GetString(export_name);
+      entry->cell_index = cell_index;
 
       AddRegularExport(entry);
     }
@@ -201,6 +206,36 @@ void ModuleDescriptor::MakeIndirectExportsExplicit(Zone* zone) {
   }
 }
 
+void ModuleDescriptor::AssignCellIndices() {
+  int export_index = 1;
+  for (auto it = regular_exports_.begin(); it != regular_exports_.end();) {
+    auto current_key = it->first;
+    // This local name may be exported under multiple export names.  Assign the
+    // same index to each such entry.
+    do {
+      Entry* entry = it->second;
+      DCHECK_NOT_NULL(entry->local_name);
+      DCHECK_NULL(entry->import_name);
+      DCHECK_LT(entry->module_request, 0);
+      DCHECK_EQ(entry->cell_index, 0);
+      entry->cell_index = export_index;
+      it++;
+    } while (it != regular_exports_.end() && it->first == current_key);
+    export_index++;
+  }
+
+  int import_index = -1;
+  for (const auto& elem : regular_imports_) {
+    Entry* entry = elem.second;
+    DCHECK_NOT_NULL(entry->local_name);
+    DCHECK_NOT_NULL(entry->import_name);
+    DCHECK_LE(0, entry->module_request);
+    DCHECK_EQ(entry->cell_index, 0);
+    entry->cell_index = import_index;
+    import_index--;
+  }
+}
+
 namespace {
 
 const ModuleDescriptor::Entry* BetterDuplicate(
@@ -267,6 +302,7 @@ bool ModuleDescriptor::Validate(ModuleScope* module_scope,
   }
 
   MakeIndirectExportsExplicit(zone);
+  AssignCellIndices();
   return true;
 }
 

src/ast/modules.h

@@ -77,19 +77,28 @@ class ModuleDescriptor : public ZoneObject {
     const AstRawString* export_name;
     const AstRawString* local_name;
     const AstRawString* import_name;
+
     // The module_request value records the order in which modules are
     // requested. It also functions as an index into the ModuleInfo's array of
     // module specifiers and into the Module's array of requested modules.  A
     // negative value means no module request.
     int module_request;
 
+    // Import/export entries that are associated with a MODULE-allocated
+    // variable use the cell_index value to encode the location of their cell.
+    // Negative values are used for imports and positive values for exports.
+    // For entries that are not associated with a MODULE-allocated variable,
+    // cell_index is 0.
+    int cell_index;
+
     // TODO(neis): Remove local_name component?
     explicit Entry(Scanner::Location loc)
         : location(loc),
           export_name(nullptr),
           local_name(nullptr),
           import_name(nullptr),
-          module_request(-1) {}
+          module_request(-1),
+          cell_index(0) {}
 
     // (De-)serialization support.
     // Note that the location value is not preserved as it's only needed by the
@@ -110,7 +119,7 @@ class ModuleDescriptor : public ZoneObject {
   }
 
   // All the remaining imports, indexed by local name.
-  const ZoneMap<const AstRawString*, const Entry*>& regular_imports() const {
+  const ZoneMap<const AstRawString*, Entry*>& regular_imports() const {
     return regular_imports_;
   }
 
@@ -139,7 +148,7 @@ class ModuleDescriptor : public ZoneObject {
     special_exports_.Add(entry, zone);
   }
 
-  void AddRegularImport(const Entry* entry) {
+  void AddRegularImport(Entry* entry) {
     DCHECK_NOT_NULL(entry->import_name);
     DCHECK_NOT_NULL(entry->local_name);
     DCHECK_NULL(entry->export_name);
@@ -168,7 +177,7 @@ class ModuleDescriptor : public ZoneObject {
   ZoneList<const Entry*> special_exports_;
   ZoneList<const Entry*> namespace_imports_;
   ZoneMultimap<const AstRawString*, Entry*> regular_exports_;
-  ZoneMap<const AstRawString*, const Entry*> regular_imports_;
+  ZoneMap<const AstRawString*, Entry*> regular_imports_;
 
   // If there are multiple export entries with the same export name, return the
   // last of them (in source order).  Otherwise return nullptr.
@@ -192,6 +201,11 @@ class ModuleDescriptor : public ZoneObject {
   // (The import entry is never deleted.)
   void MakeIndirectExportsExplicit(Zone* zone);
 
+  // Assign a cell_index of -1,-2,... to regular imports.
+  // Assign a cell_index of +1,+2,... to regular (local) exports.
+  // Assign a cell_index of 0 to anything else.
+  void AssignCellIndices();
+
   int AddModuleRequest(const AstRawString* specifier) {
     DCHECK_NOT_NULL(specifier);
     auto it = module_requests_

src/ast/scopeinfo.cc

@@ -651,7 +651,7 @@ int ScopeInfo::ModuleIndex(Handle<String> name, VariableMode* mode,
     entry += kModuleVariableEntryLength;
   }
 
-  return -1;
+  return 0;
 }
 
 int ScopeInfo::ContextSlotIndex(Handle<ScopeInfo> scope_info,
@@ -803,6 +803,7 @@ void ScopeInfo::ModuleVariable(int i, String** name, int* index,
   }
   if (index != nullptr) {
     *index = Smi::cast(get(entry + kModuleVariableIndexOffset))->value();
+    DCHECK_NE(*index, 0);
   }
   if (mode != nullptr) {
     *mode = VariableModeField::decode(properties);
@@ -864,13 +865,14 @@ Handle<ModuleInfoEntry> ModuleInfoEntry::New(Isolate* isolate,
                                              Handle<Object> export_name,
                                              Handle<Object> local_name,
                                              Handle<Object> import_name,
-                                             Handle<Object> module_request,
+                                             int module_request, int cell_index,
                                              int beg_pos, int end_pos) {
   Handle<ModuleInfoEntry> result = isolate->factory()->NewModuleInfoEntry();
   result->set(kExportNameIndex, *export_name);
   result->set(kLocalNameIndex, *local_name);
   result->set(kImportNameIndex, *import_name);
-  result->set(kModuleRequestIndex, *module_request);
+  result->set(kModuleRequestIndex, Smi::FromInt(module_request));
+  result->set(kCellIndexIndex, Smi::FromInt(cell_index));
   result->set(kBegPosIndex, Smi::FromInt(beg_pos));
   result->set(kEndPosIndex, Smi::FromInt(end_pos));
   return result;

src/ast/scopes.cc

@@ -779,20 +779,29 @@ Variable* Scope::LookupInScopeInfo(const AstRawString* name) {
   // There should be no local slot with the given name.
   DCHECK_LT(scope_info_->StackSlotIndex(*name_handle), 0);
 
+  bool found = false;
+
+  VariableLocation location;
+  int index;
   VariableMode mode;
   InitializationFlag init_flag;
   MaybeAssignedFlag maybe_assigned_flag;
 
-  VariableLocation location = VariableLocation::CONTEXT;
-  int index = ScopeInfo::ContextSlotIndex(scope_info_, name_handle, &mode,
-                                          &init_flag, &maybe_assigned_flag);
-  if (index < 0 && scope_type() == MODULE_SCOPE) {
+  {
+    location = VariableLocation::CONTEXT;
+    index = ScopeInfo::ContextSlotIndex(scope_info_, name_handle, &mode,
+                                        &init_flag, &maybe_assigned_flag);
+    found = index >= 0;
+  }
+
+  if (!found && scope_type() == MODULE_SCOPE) {
     location = VariableLocation::MODULE;
     index = scope_info_->ModuleIndex(name_handle, &mode, &init_flag,
                                      &maybe_assigned_flag);
+    found = index != 0;
   }
 
-  if (index < 0) {
+  if (!found) {
     index = scope_info_->FunctionContextSlotIndex(*name_handle);
     if (index < 0) return nullptr;  // Nowhere found.
     Variable* var = AsDeclarationScope()->DeclareFunctionVar(name);
@@ -1936,12 +1945,14 @@ void DeclarationScope::AllocateLocals() {
 void ModuleScope::AllocateModuleVariables() {
   for (const auto& it : module()->regular_imports()) {
     Variable* var = LookupLocal(it.first);
-    var->AllocateTo(VariableLocation::MODULE, Variable::kModuleImportIndex);
+    var->AllocateTo(VariableLocation::MODULE, it.second->cell_index);
+    DCHECK(!var->IsExport());
   }
 
   for (const auto& it : module()->regular_exports()) {
     Variable* var = LookupLocal(it.first);
-    var->AllocateTo(VariableLocation::MODULE, Variable::kModuleExportIndex);
+    var->AllocateTo(VariableLocation::MODULE, it.second->cell_index);
+    DCHECK(var->IsExport());
   }
 }
 

src/ast/variables.h

@@ -100,19 +100,16 @@ class Variable final : public ZoneObject {
 
   int index() const { return index_; }
 
-  // Sentinel index values for module exports and imports.
-  enum { kModuleExportIndex, kModuleImportIndex };
-
   bool IsExport() const {
-    DCHECK(location() == VariableLocation::MODULE);
-    return index() == kModuleExportIndex;
+    DCHECK_EQ(location(), VariableLocation::MODULE);
+    DCHECK_NE(index(), 0);
+    return index() > 0;
   }
 
   void AllocateTo(VariableLocation location, int index) {
     DCHECK(IsUnallocated() ||
            (this->location() == location && this->index() == index));
-    DCHECK_IMPLIES(location == VariableLocation::MODULE,
-                   index == kModuleExportIndex || index == kModuleImportIndex);
+    DCHECK_IMPLIES(location == VariableLocation::MODULE, index != 0);
     bit_field_ = LocationField::update(bit_field_, location);
     DCHECK_EQ(location, this->location());
     index_ = index;

src/debug/debug-scopes.cc

@@ -801,7 +801,8 @@ void ScopeIterator::CopyModuleVarsToScopeObject(Handle<ScopeInfo> scope_info,
       scope_info->ModuleVariable(i, &name, &index);
       CHECK(!ScopeInfo::VariableIsSynthetic(name));
       local_name = handle(name, isolate);
-      is_export = index == Variable::kModuleExportIndex;
+      DCHECK_NE(index, 0);
+      is_export = index > 0;
     }
 
     Handle<Object> value;
@@ -814,9 +815,8 @@ void ScopeIterator::CopyModuleVarsToScopeObject(Handle<ScopeInfo> scope_info,
       Handle<ModuleInfoEntry> entry =
           ModuleInfo::LookupRegularImport(module_info, local_name);
       Handle<String> import_name(String::cast(entry->import_name()), isolate);
-      int module_request = Smi::cast(entry->module_request())->value();
       value = Module::LoadImport(handle(context->module(), isolate),
-                                 import_name, module_request);
+                                 import_name, entry->module_request());
     }
 
     // Reflect variables under TDZ as undefined in scope object.

src/objects-inl.h

@@ -8052,8 +8052,12 @@ Object* ModuleInfoEntry::local_name() const { return get(kLocalNameIndex); }
 
 Object* ModuleInfoEntry::import_name() const { return get(kImportNameIndex); }
 
-Object* ModuleInfoEntry::module_request() const {
-  return get(kModuleRequestIndex);
+int ModuleInfoEntry::module_request() const {
+  return Smi::cast(get(kModuleRequestIndex))->value();
+}
+
+int ModuleInfoEntry::cell_index() const {
+  return Smi::cast(get(kCellIndexIndex))->value();
 }
 
 int ModuleInfoEntry::beg_pos() const {

src/objects.cc

@@ -19912,7 +19912,6 @@ MaybeHandle<Cell> Module::ResolveExport(Handle<Module> module,
   if (object->IsModuleInfoEntry()) {
     // Not yet resolved indirect export.
     Handle<ModuleInfoEntry> entry = Handle<ModuleInfoEntry>::cast(object);
-    int module_request = Smi::cast(entry->module_request())->value();
     Handle<String> import_name(String::cast(entry->import_name()), isolate);
     Handle<Script> script(
         Script::cast(JSFunction::cast(module->code())->shared()->script()),
@@ -19920,8 +19919,8 @@ MaybeHandle<Cell> Module::ResolveExport(Handle<Module> module,
     MessageLocation new_loc(script, entry->beg_pos(), entry->end_pos());
 
     Handle<Cell> cell;
-    if (!ResolveImport(module, import_name, module_request, new_loc, true,
-                       resolve_set)
+    if (!ResolveImport(module, import_name, entry->module_request(), new_loc,
+                       true, resolve_set)
              .ToHandle(&cell)) {
       DCHECK(isolate->has_pending_exception());
       return MaybeHandle<Cell>();
@@ -19958,7 +19957,6 @@ MaybeHandle<Cell> Module::ResolveExportUsingStarExports(
       if (!entry->export_name()->IsUndefined(isolate)) {
         continue;  // Indirect export.
       }
-      int module_request = Smi::cast(entry->module_request())->value();
 
       Handle<Script> script(
           Script::cast(JSFunction::cast(module->code())->shared()->script()),
@@ -19966,7 +19964,7 @@ MaybeHandle<Cell> Module::ResolveExportUsingStarExports(
       MessageLocation new_loc(script, entry->beg_pos(), entry->end_pos());
 
       Handle<Cell> cell;
-      if (ResolveImport(module, name, module_request, new_loc, false,
+      if (ResolveImport(module, name, entry->module_request(), new_loc, false,
                         resolve_set)
               .ToHandle(&cell)) {
         if (unique_cell.is_null()) unique_cell = cell;
@@ -20019,9 +20017,10 @@ bool Module::Instantiate(Handle<Module> module, v8::Local<v8::Context> context,
 
   // Set up local exports.
   Handle<FixedArray> regular_exports(module_info->regular_exports(), isolate);
-  for (int i = 0, n = regular_exports->length(); i < n; i += 2) {
+  for (int i = 0, n = regular_exports->length(); i < n; i += 3) {
+    // TODO(neis): Make this more robust.
     Handle<FixedArray> export_names(
-        FixedArray::cast(regular_exports->get(i + 1)), isolate);
+        FixedArray::cast(regular_exports->get(i + 2)), isolate);
     CreateExport(module, export_names);
   }
 
@@ -20069,13 +20068,13 @@ bool Module::Instantiate(Handle<Module> module, v8::Local<v8::Context> context,
     Handle<ModuleInfoEntry> entry(
         ModuleInfoEntry::cast(regular_imports->get(i)), isolate);
     Handle<String> name(String::cast(entry->import_name()), isolate);
-    int module_request = Smi::cast(entry->module_request())->value();
     Handle<Script> script(
         Script::cast(JSFunction::cast(module->code())->shared()->script()),
         isolate);
     MessageLocation loc(script, entry->beg_pos(), entry->end_pos());
     ResolveSet resolve_set(&zone);
-    if (ResolveImport(module, name, module_request, loc, true, &resolve_set)
+    if (ResolveImport(module, name, entry->module_request(), loc, true,
+                      &resolve_set)
             .is_null()) {
       return false;
     }
@@ -20159,9 +20158,8 @@ void FetchStarExports(Handle<Module> module, Zone* zone,
       continue;  // Indirect export.
     }
 
-    int module_request = Smi::cast(entry->module_request())->value();
     Handle<Module> requested_module(
-        Module::cast(module->requested_modules()->get(module_request)),
+        Module::cast(module->requested_modules()->get(entry->module_request())),
         isolate);
 
     // Recurse.

src/objects.h

@@ -4546,8 +4546,9 @@ class ScopeInfo : public FixedArray {
                               VariableMode* mode, InitializationFlag* init_flag,
                               MaybeAssignedFlag* maybe_assigned_flag);
 
-  // Lookup metadata of a MODULE-allocated variable.  Return a negative value if
-  // there is no module variable with the given name.
+  // Lookup metadata of a MODULE-allocated variable.  Return 0 if there is no
+  // module variable with the given name (the index value of a MODULE variable
+  // is never 0).
   int ModuleIndex(Handle<String> name, VariableMode* mode,
                   InitializationFlag* init_flag,
                   MaybeAssignedFlag* maybe_assigned_flag);
@@ -4740,12 +4741,13 @@ class ModuleInfoEntry : public FixedArray {
                                      Handle<Object> export_name,
                                      Handle<Object> local_name,
                                      Handle<Object> import_name,
-                                     Handle<Object> module_request, int beg_pos,
-                                     int end_pos);
+                                     int module_request, int cell_index,
+                                     int beg_pos, int end_pos);
   inline Object* export_name() const;
   inline Object* local_name() const;
   inline Object* import_name() const;
-  inline Object* module_request() const;
+  inline int module_request() const;
+  inline int cell_index() const;
   inline int beg_pos() const;
   inline int end_pos() const;
 
@@ -4756,6 +4758,7 @@ class ModuleInfoEntry : public FixedArray {
     kLocalNameIndex,
     kImportNameIndex,
     kModuleRequestIndex,
+    kCellIndexIndex,
     kBegPosIndex,
     kEndPosIndex,
     kLength
@@ -4763,6 +4766,7 @@ class ModuleInfoEntry : public FixedArray {
 };
 
 // ModuleInfo is to ModuleDescriptor what ScopeInfo is to Scope.
+// TODO(neis): Use Struct instead of FixedArray.
 class ModuleInfo : public FixedArray {
  public:
   DECLARE_CAST(ModuleInfo)