[modules] Assign cell indices at validation time.

src/ast/modules.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/ast/modules.h"
 #include "src/ast/ast-value-factory.h"
 #include "src/ast/scopes.h"
 
 namespace v8 {
 namespace internal {
@@ skipping 69 matching lines @@
 }
 
 }  // namespace
 
 Handle<ModuleInfoEntry> ModuleDescriptor::Entry::Serialize(
     Isolate* isolate) const {
   CHECK(Smi::IsValid(module_request));  // TODO(neis): Check earlier?
   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(
     Isolate* isolate, AstValueFactory* avfactory,
     Handle<ModuleInfoEntry> entry) {
   Entry* result = new (avfactory->zone()) Entry(Scanner::Location::invalid());
   result->export_name = FromStringOrUndefined(
       isolate, avfactory, handle(entry->export_name(), isolate));
   result->local_name = FromStringOrUndefined(
       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;
 }
 
 Handle<FixedArray> ModuleDescriptor::SerializeRegularExports(Isolate* isolate,
                                                              Zone* zone) const {
   // We serialize regular exports in a way that lets us later iterate over their
   // local names and for each local name immediately access all its export
   // 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.
     int i = 0;
     for (; it != next; ++it) {
       export_names->set(i++, *it->second->export_name->string());
     }
     DCHECK_EQ(i, size);
 
     // Continue with the next distinct key.
     DCHECK(it == next);
   }
 
   // We cannot create the FixedArray earlier because we only now know the
   // precise size (the number of unique keys in regular_exports).
   int size = static_cast<int>(data.size());
   Handle<FixedArray> result = isolate->factory()->NewFixedArray(size);
   for (int i = 0; i < size; ++i) {
     result->set(i, *data[i]);
   }
   return result;
 }
 
 void ModuleDescriptor::DeserializeRegularExports(Isolate* isolate,
                                                  AstValueFactory* avfactory,
                                                  Handle<FixedArray> data) {
   for (int i = 0, length_i = data->length(); i < length_i;) {

[adamk, 2016/10/31 18:25:04]

Either this should have a DCHECK that data->length() % 2 == 0, or the loop
should be bounded on i+1

[neis, 2016/11/03 10:43:47]

Now it should be modulo 3. Yeah that's part of the TODO about making the regular
exports handling more robust. (Not sure what you mean by bounded on i+1.)

     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) {
       Handle<String> export_name(String::cast(export_names->get(j)), isolate);
 
       Entry* entry =
           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);
     }
   }
 }
 
 void ModuleDescriptor::MakeIndirectExportsExplicit(Zone* zone) {
   for (auto it = regular_exports_.begin(); it != regular_exports_.end();) {
     Entry* entry = it->second;
     DCHECK_NOT_NULL(entry->local_name);
@@ skipping 18 matching lines @@
       entry->location = import->second->location;
       entry->local_name = nullptr;
       AddSpecialExport(entry, zone);
       it = regular_exports_.erase(it);
     } else {
       it++;
     }
   }
 }
 
+void ModuleDescriptor::AssignCellIndices() {
+  int export_index = 1;
+  for (auto it = regular_exports_.begin(); it != regular_exports_.end();) {
+    auto current_key = it->first;
+    do {

[adamk, 2016/10/31 18:25:04]

Please add a comment for this loop, I expect it will be obscure to future code
archaeologists.

[neis, 2016/11/03 10:43:47]

Done.

+      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);

[adamk, 2016/10/31 18:25:04]

This 0 now appears three times, perhaps it should have a constant that refers to
it?

[neis, 2016/11/03 10:43:48]

I'd like to keep it just 0 for now but it's on my list of things to get back to.

+    entry->cell_index = import_index;
+    import_index--;
+  }
+}
+
 namespace {
 
 const ModuleDescriptor::Entry* BetterDuplicate(
     const ModuleDescriptor::Entry* candidate,
     ZoneMap<const AstRawString*, const ModuleDescriptor::Entry*>& export_names,
     const ModuleDescriptor::Entry* current_duplicate) {
   DCHECK_NOT_NULL(candidate->export_name);
   DCHECK(candidate->location.IsValid());
   auto insert_result =
       export_names.insert(std::make_pair(candidate->export_name, candidate));
@@ skipping 46 matching lines @@
     DCHECK_NOT_NULL(entry->local_name);
     if (module_scope->LookupLocal(entry->local_name) == nullptr) {
       error_handler->ReportMessageAt(
           entry->location.beg_pos, entry->location.end_pos,
           MessageTemplate::kModuleExportUndefined, entry->local_name);
       return false;
     }
   }
 
   MakeIndirectExportsExplicit(zone);
+  AssignCellIndices();
   return true;
 }
 
 }  // namespace internal
 }  // namespace v8