[wasm] Move decoded local names off-heap

We were decoding the names of locals into a C++ data structure, and then
generated a FixedArray out of that, stored in the on-heap WasmDebugInfo.
In order to support name lookup for debugging with Liftoff, where no
WasmDebugInfo will be present, this CL refactors the C++ data structure
to allow direct lookups and stores it in the C++ DebugInfo structure.

With this CL, the names are still only used from the old
interpreter-based debugging path. A follow-up CL will then also use it
from Liftoff.

R=thibaudm@chromium.org

Bug: v8:10019
Change-Id: I1397021b5d69b9346fc26f5e83653360f428c5e7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2002541
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65819}

src/wasm/module-decoder.cc

@@ -2171,14 +2171,11 @@ void DecodeFunctionNames(const byte* module_start, const byte* module_end,
   }
 }
 
-void DecodeLocalNames(const byte* module_start, const byte* module_end,
-                      LocalNames* result) {
-  DCHECK_NOT_NULL(result);
-  DCHECK(result->names.empty());
-
-  Decoder decoder(module_start, module_end);
-  if (!FindNameSection(&decoder)) return;
+LocalNames DecodeLocalNames(Vector<const uint8_t> module_bytes) {
+  Decoder decoder(module_bytes);
+  if (!FindNameSection(&decoder)) return LocalNames{{}};
 
+  std::vector<LocalNamesPerFunction> functions;
   while (decoder.ok() && decoder.more()) {
     uint8_t name_type = decoder.consume_u8("name type");
     if (name_type & 0x80) break;  // no varuint7
@@ -2195,22 +2192,26 @@ void DecodeLocalNames(const byte* module_start, const byte* module_end,
     for (uint32_t i = 0; i < local_names_count; ++i) {
       uint32_t func_index = decoder.consume_u32v("function index");
       if (func_index > kMaxInt) continue;
-      result->names.emplace_back(static_cast<int>(func_index));
-      LocalNamesPerFunction& func_names = result->names.back();
-      result->max_function_index =
-          std::max(result->max_function_index, func_names.function_index);
+      std::vector<LocalName> names;
       uint32_t num_names = decoder.consume_u32v("namings count");
       for (uint32_t k = 0; k < num_names; ++k) {
         uint32_t local_index = decoder.consume_u32v("local index");
-        WireBytesRef name = consume_string(&decoder, true, "local name");
+        WireBytesRef name = consume_string(&decoder, false, "local name");
         if (!decoder.ok()) break;
         if (local_index > kMaxInt) continue;
-        func_names.max_local_index =
-            std::max(func_names.max_local_index, static_cast<int>(local_index));
-        func_names.names.emplace_back(static_cast<int>(local_index), name);
+        // Ignore non-utf8 names.
+        if (!validate_utf8(&decoder, name)) continue;
+        names.emplace_back(static_cast<int>(local_index), name);
       }
+      // Use stable sort to get deterministic names (the first one declared)
+      // even in the presence of duplicates.
+      std::stable_sort(names.begin(), names.end(), LocalName::IndexLess{});
+      functions.emplace_back(static_cast<int>(func_index), std::move(names));
     }
   }
+  std::stable_sort(functions.begin(), functions.end(),
+                   LocalNamesPerFunction::FunctionIndexLess{});
+  return LocalNames{std::move(functions)};
 }
 
 #undef TRACE

src/wasm/module-decoder.h

@@ -42,22 +42,80 @@ struct AsmJsOffsetEntry {
 using AsmJsOffsets = std::vector<std::vector<AsmJsOffsetEntry>>;
 using AsmJsOffsetsResult = Result<AsmJsOffsets>;
 
-struct LocalName {
-  int local_index;
-  WireBytesRef name;
-  LocalName(int local_index, WireBytesRef name)
-      : local_index(local_index), name(name) {}
+class LocalName {
+ public:
+  LocalName(int index, WireBytesRef name) : index_(index), name_(name) {}
+
+  int index() const { return index_; }
+  WireBytesRef name() const { return name_; }
+
+  struct IndexLess {
+    bool operator()(const LocalName& a, const LocalName& b) const {
+      return a.index() < b.index();
+    }
+  };
+
+ private:
+  int index_;
+  WireBytesRef name_;
 };
-struct LocalNamesPerFunction {
-  int function_index;
-  int max_local_index = -1;
-  std::vector<LocalName> names;
-  explicit LocalNamesPerFunction(int function_index)
-      : function_index(function_index) {}
+
+class LocalNamesPerFunction {
+ public:
+  // For performance reasons, {LocalNamesPerFunction} should not be copied.
+  MOVE_ONLY_NO_DEFAULT_CONSTRUCTOR(LocalNamesPerFunction);
+
+  LocalNamesPerFunction(int function_index, std::vector<LocalName> names)
+      : function_index_(function_index), names_(std::move(names)) {
+    DCHECK(
+        std::is_sorted(names_.begin(), names_.end(), LocalName::IndexLess{}));
+  }
+
+  int function_index() const { return function_index_; }
+
+  WireBytesRef GetName(int local_index) {
+    auto it =
+        std::lower_bound(names_.begin(), names_.end(),
+                         LocalName{local_index, {}}, LocalName::IndexLess{});
+    if (it == names_.end()) return {};
+    if (it->index() != local_index) return {};
+    return it->name();
+  }
+
+  struct FunctionIndexLess {
+    bool operator()(const LocalNamesPerFunction& a,
+                    const LocalNamesPerFunction& b) const {
+      return a.function_index() < b.function_index();
+    }
+  };
+
+ private:
+  int function_index_;
+  std::vector<LocalName> names_;
 };
-struct LocalNames {
-  int max_function_index = -1;
-  std::vector<LocalNamesPerFunction> names;
+
+class LocalNames {
+ public:
+  // For performance reasons, {LocalNames} should not be copied.
+  MOVE_ONLY_NO_DEFAULT_CONSTRUCTOR(LocalNames);
+
+  explicit LocalNames(std::vector<LocalNamesPerFunction> functions)
+      : functions_(std::move(functions)) {
+    DCHECK(std::is_sorted(functions_.begin(), functions_.end(),
+                          LocalNamesPerFunction::FunctionIndexLess{}));
+  }
+
+  WireBytesRef GetName(int function_index, int local_index) {
+    auto it = std::lower_bound(functions_.begin(), functions_.end(),
+                               LocalNamesPerFunction{function_index, {}},
+                               LocalNamesPerFunction::FunctionIndexLess{});
+    if (it == functions_.end()) return {};
+    if (it->function_index() != function_index) return {};
+    return it->GetName(local_index);
+  }
+
+ private:
+  std::vector<LocalNamesPerFunction> functions_;
 };
 
 // Decodes the bytes of a wasm module between {module_start} and {module_end}.
@@ -106,11 +164,10 @@ void DecodeFunctionNames(const byte* module_start, const byte* module_end,
                          std::unordered_map<uint32_t, WireBytesRef>* names);
 
 // Decode the local names assignment from the name section.
-// Stores the result in the given {LocalNames} structure. The result will be
-// empty if no name section is present. On encountering an error in the name
-// section, returns all information decoded up to the first error.
-void DecodeLocalNames(const byte* module_start, const byte* module_end,
-                      LocalNames* result);
+// The result will be empty if no name section is present. On encountering an
+// error in the name section, returns all information decoded up to the first
+// error.
+LocalNames DecodeLocalNames(Vector<const uint8_t> module_bytes);
 
 class ModuleDecoderImpl;
 

src/wasm/wasm-debug.cc

@@ -72,31 +72,17 @@ Handle<Object> WasmValueToValueObject(Isolate* isolate, WasmValue value) {
   }
 }
 
-MaybeHandle<String> GetLocalName(Isolate* isolate,
-                                 Handle<WasmDebugInfo> debug_info,
-                                 int func_index, int local_index) {
-  DCHECK_LE(0, func_index);
-  DCHECK_LE(0, local_index);
-  if (!debug_info->has_locals_names()) {
-    Handle<WasmModuleObject> module_object(
-        debug_info->wasm_instance().module_object(), isolate);
-    Handle<FixedArray> locals_names = DecodeLocalNames(isolate, module_object);
-    debug_info->set_locals_names(*locals_names);
-  }
-
-  Handle<FixedArray> locals_names(debug_info->locals_names(), isolate);
-  if (func_index >= locals_names->length() ||
-      locals_names->get(func_index).IsUndefined(isolate)) {
-    return {};
-  }
-
-  Handle<FixedArray> func_locals_names(
-      FixedArray::cast(locals_names->get(func_index)), isolate);
-  if (local_index >= func_locals_names->length() ||
-      func_locals_names->get(local_index).IsUndefined(isolate)) {
-    return {};
-  }
-  return handle(String::cast(func_locals_names->get(local_index)), isolate);
+MaybeHandle<String> GetLocalNameString(Isolate* isolate,
+                                       NativeModule* native_module,
+                                       int func_index, int local_index) {
+  WireBytesRef name_ref =
+      native_module->GetDebugInfo()->GetLocalName(func_index, local_index);
+  ModuleWireBytes wire_bytes{native_module->wire_bytes()};
+  // Bounds were checked during decoding.
+  DCHECK(wire_bytes.BoundsCheck(name_ref));
+  Vector<const char> name = wire_bytes.GetNameOrNull(name_ref);
+  if (name.begin() == nullptr) return {};
+  return isolate->factory()->NewStringFromUtf8(name);
 }
 
 class InterpreterHandle {
@@ -407,10 +393,13 @@ class InterpreterHandle {
           isolate_->factory()->InternalizeString(StaticCharVector("locals"));
       JSObject::AddProperty(isolate, local_scope_object, locals_name,
                             locals_obj, NONE);
+      NativeModule* native_module =
+          debug_info->wasm_instance().module_object().native_module();
       for (int i = 0; i < num_locals; ++i) {
-        MaybeHandle<Name> name =
-            GetLocalName(isolate, debug_info, frame->function()->func_index, i);
-        if (name.is_null()) {
+        Handle<Name> name;
+        if (!GetLocalNameString(isolate, native_module,
+                                frame->function()->func_index, i)
+                 .ToHandle(&name)) {
           // Parameters should come before locals in alphabetical ordering, so
           // we name them "args" here.
           const char* label = i < num_params ? "arg#%d" : "local#%d";
@@ -419,7 +408,7 @@ class InterpreterHandle {
         WasmValue value = frame->GetLocalValue(i);
         Handle<Object> value_obj = WasmValueToValueObject(isolate_, value);
         // {name} can be a string representation of an element index.
-        LookupIterator::Key lookup_key{isolate, name.ToHandleChecked()};
+        LookupIterator::Key lookup_key{isolate, name};
         LookupIterator it(isolate, locals_obj, lookup_key, locals_obj,
                           LookupIterator::OWN_SKIP_INTERCEPTOR);
         if (it.IsFound()) continue;
@@ -566,6 +555,15 @@ class DebugInfoImpl {
     return local_scope_object;
   }
 
+  WireBytesRef GetLocalName(int func_index, int local_index) {
+    base::MutexGuard guard(&mutex_);
+    if (!local_names_) {
+      local_names_ = std::make_unique<LocalNames>(
+          DecodeLocalNames(native_module_->wire_bytes()));
+    }
+    return local_names_->GetName(func_index, local_index);
+  }
+
  private:
   DebugSideTable* GetDebugSideTable(AccountingAllocator* allocator,
                                     int func_index) {
@@ -623,12 +621,15 @@ class DebugInfoImpl {
 
   NativeModule* const native_module_;
 
-  // {mutex_} protects {debug_side_tables_}.
+  // {mutex_} protects {debug_side_tables_} and {local_names_}.
   base::Mutex mutex_;
 
   // DebugSideTable per function, lazily initialized.
   std::vector<std::unique_ptr<DebugSideTable>> debug_side_tables_;
 
+  // Names of locals, lazily decoded from the wire bytes.
+  std::unique_ptr<LocalNames> local_names_;
+
   DISALLOW_COPY_AND_ASSIGN(DebugInfoImpl);
 };
 
@@ -642,6 +643,10 @@ Handle<JSObject> DebugInfo::GetLocalScopeObject(Isolate* isolate, Address pc,
   return impl_->GetLocalScopeObject(isolate, pc, fp);
 }
 
+WireBytesRef DebugInfo::GetLocalName(int func_index, int local_index) {
+  return impl_->GetLocalName(func_index, local_index);
+}
+
 }  // namespace wasm
 
 namespace {

src/wasm/wasm-debug.h

@@ -26,7 +26,9 @@ class WasmInstanceObject;
 namespace wasm {
 
 class DebugInfoImpl;
+class LocalNames;
 class NativeModule;
+class WireBytesRef;
 
 // Side table storing information used to inspect Liftoff frames at runtime.
 // This table is only created on demand for debugging, so it is not optimized
@@ -147,6 +149,8 @@ class DebugInfo {
 
   Handle<JSObject> GetLocalScopeObject(Isolate*, Address pc, Address fp);
 
+  WireBytesRef GetLocalName(int func_index, int local_index);
+
  private:
   std::unique_ptr<DebugInfoImpl> impl_;
 };

src/wasm/wasm-module.cc

@@ -132,7 +132,7 @@ void DecodedFunctionNames::AddForTesting(int function_index,
 // Get a string stored in the module bytes representing a name.
 WasmName ModuleWireBytes::GetNameOrNull(WireBytesRef ref) const {
   if (!ref.is_set()) return {nullptr, 0};  // no name.
-  CHECK(BoundsCheck(ref.offset(), ref.length()));
+  DCHECK(BoundsCheck(ref));
   return WasmName::cast(
       module_bytes_.SubVector(ref.offset(), ref.end_offset()));
 }
@@ -569,28 +569,6 @@ Handle<JSArray> GetCustomSections(Isolate* isolate,
   return array_object;
 }
 
-Handle<FixedArray> DecodeLocalNames(Isolate* isolate,
-                                    Handle<WasmModuleObject> module_object) {
-  Vector<const uint8_t> wire_bytes =
-      module_object->native_module()->wire_bytes();
-  LocalNames decoded_locals;
-  DecodeLocalNames(wire_bytes.begin(), wire_bytes.end(), &decoded_locals);
-  Handle<FixedArray> locals_names =
-      isolate->factory()->NewFixedArray(decoded_locals.max_function_index + 1);
-  for (LocalNamesPerFunction& func : decoded_locals.names) {
-    Handle<FixedArray> func_locals_names =
-        isolate->factory()->NewFixedArray(func.max_local_index + 1);
-    locals_names->set(func.function_index, *func_locals_names);
-    for (LocalName& name : func.names) {
-      Handle<String> name_str =
-          WasmModuleObject::ExtractUtf8StringFromModuleBytes(
-              isolate, module_object, name.name, kNoInternalize);
-      func_locals_names->set(name.local_index, *name_str);
-    }
-  }
-  return locals_names;
-}
-
 namespace {
 template <typename T>
 inline size_t VectorSize(const std::vector<T>& vector) {

src/wasm/wasm-module.h

@@ -296,10 +296,10 @@ struct V8_EXPORT_PRIVATE ModuleWireBytes {
   WasmName GetNameOrNull(const WasmFunction* function,
                          const WasmModule* module) const;
 
-  // Checks the given offset range is contained within the module bytes.
-  bool BoundsCheck(uint32_t offset, uint32_t length) const {
+  // Checks the given reference is contained within the module bytes.
+  bool BoundsCheck(WireBytesRef ref) const {
     uint32_t size = static_cast<uint32_t>(module_bytes_.length());
-    return offset <= size && length <= size - offset;
+    return ref.offset() <= size && ref.length() <= size - ref.offset();
   }
 
   Vector<const byte> GetFunctionBytes(const WasmFunction* function) const {
@@ -344,11 +344,6 @@ Handle<JSArray> GetCustomSections(Isolate* isolate,
                                   Handle<WasmModuleObject> module,
                                   Handle<String> name, ErrorThrower* thrower);
 
-// Decode local variable names from the names section. Return FixedArray of
-// FixedArray of <undefined|String>. The outer fixed array is indexed by the
-// function index, the inner one by the local index.
-Handle<FixedArray> DecodeLocalNames(Isolate*, Handle<WasmModuleObject>);
-
 // TruncatedUserString makes it easy to output names up to a certain length, and
 // output a truncation followed by '...' if they exceed a limit.
 // Use like this:

src/wasm/wasm-objects-inl.h

@@ -388,7 +388,6 @@ ACCESSORS(WasmDebugInfo, wasm_instance, WasmInstanceObject, kInstanceOffset)
 ACCESSORS(WasmDebugInfo, interpreter_handle, Object, kInterpreterHandleOffset)
 ACCESSORS(WasmDebugInfo, interpreter_reference_stack, Cell,
           kInterpreterReferenceStackOffset)
-OPTIONAL_ACCESSORS(WasmDebugInfo, locals_names, FixedArray, kLocalsNamesOffset)
 OPTIONAL_ACCESSORS(WasmDebugInfo, c_wasm_entries, FixedArray,
                    kCWasmEntriesOffset)
 OPTIONAL_ACCESSORS(WasmDebugInfo, c_wasm_entry_map, Managed<wasm::SignatureMap>,

src/wasm/wasm-objects.h

@@ -813,7 +813,6 @@ class WasmDebugInfo : public Struct {
   DECL_ACCESSORS(wasm_instance, WasmInstanceObject)
   DECL_ACCESSORS(interpreter_handle, Object)  // Foreign or undefined
   DECL_ACCESSORS(interpreter_reference_stack, Cell)
-  DECL_OPTIONAL_ACCESSORS(locals_names, FixedArray)
   DECL_OPTIONAL_ACCESSORS(c_wasm_entries, FixedArray)
   DECL_OPTIONAL_ACCESSORS(c_wasm_entry_map, Managed<wasm::SignatureMap>)