[wasm][debug] Store debug side table per code object

The debug side table is indexed by pc offset. Offsets change if
breakpoints are added or removed, hence we cannot reuse the debug side
table when compiling another version of the function (with a different
set of breakpoints). Thus store the debug side table per code object
instead of per function.

R=thibaudm@chromium.org

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

src/wasm/baseline/liftoff-compiler.cc

@@ -214,7 +214,7 @@ class DebugSideTableBuilder {
     local_stack_offsets_.push_back(stack_offset);
   }
 
-  DebugSideTable GenerateDebugSideTable() {
+  std::unique_ptr<DebugSideTable> GenerateDebugSideTable() {
     std::vector<DebugSideTable::Entry> table_entries;
     table_entries.reserve(entries_.size());
     for (auto& entry : entries_) table_entries.push_back(entry.ToTableEntry());
@@ -222,9 +222,9 @@ class DebugSideTableBuilder {
               [](DebugSideTable::Entry& a, DebugSideTable::Entry& b) {
                 return a.pc_offset() < b.pc_offset();
               });
-    return DebugSideTable{std::move(local_types_),
-                          std::move(local_stack_offsets_),
-                          std::move(table_entries)};
+    return std::make_unique<DebugSideTable>(std::move(local_types_),
+                                            std::move(local_stack_offsets_),
+                                            std::move(table_entries));
   }
 
  private:
@@ -2523,7 +2523,8 @@ class LiftoffCompiler {
 WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator* allocator, CompilationEnv* env,
     const FunctionBody& func_body, int func_index, Counters* counters,
-    WasmFeatures* detected, Vector<int> breakpoints) {
+    WasmFeatures* detected, Vector<int> breakpoints,
+    std::unique_ptr<DebugSideTable>* debug_sidetable) {
   int func_body_size = static_cast<int>(func_body.end - func_body.start);
   TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("v8.wasm"),
                "ExecuteLiftoffCompilation", "func_index", func_index,
@@ -2541,11 +2542,16 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   // generation.
   std::unique_ptr<wasm::WasmInstructionBuffer> instruction_buffer =
       wasm::WasmInstructionBuffer::New(128 + code_size_estimate * 4 / 3);
-  DebugSideTableBuilder* const kNoDebugSideTable = nullptr;
+  std::unique_ptr<DebugSideTableBuilder> debug_sidetable_builder;
+  // If we are emitting breakpoints, we should also emit the debug side table.
+  DCHECK_IMPLIES(!breakpoints.empty(), debug_sidetable != nullptr);
+  if (debug_sidetable) {
+    debug_sidetable_builder = std::make_unique<DebugSideTableBuilder>();
+  }
   WasmFullDecoder<Decoder::kValidate, LiftoffCompiler> decoder(
       &zone, env->module, env->enabled_features, detected, func_body,
       call_descriptor, env, &zone, instruction_buffer->CreateView(),
-      kNoDebugSideTable, breakpoints);
+      debug_sidetable_builder.get(), breakpoints);
   decoder.Decode();
   liftoff_compile_time_scope.reset();
   LiftoffCompiler* compiler = &decoder.interface();
@@ -2578,14 +2584,17 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   result.frame_slot_count = compiler->GetTotalFrameSlotCount();
   result.tagged_parameter_slots = call_descriptor->GetTaggedParameterSlots();
   result.result_tier = ExecutionTier::kLiftoff;
+  if (debug_sidetable) {
+    *debug_sidetable = debug_sidetable_builder->GenerateDebugSideTable();
+  }
 
   DCHECK(result.succeeded());
   return result;
 }
 
-DebugSideTable GenerateLiftoffDebugSideTable(AccountingAllocator* allocator,
-                                             CompilationEnv* env,
-                                             const FunctionBody& func_body) {
+std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
+    AccountingAllocator* allocator, CompilationEnv* env,
+    const FunctionBody& func_body) {
   Zone zone(allocator, "LiftoffDebugSideTableZone");
   auto call_descriptor = compiler::GetWasmCallDescriptor(&zone, func_body.sig);
   DebugSideTableBuilder debug_sidetable_builder;

src/wasm/baseline/liftoff-compiler.h

@@ -54,9 +54,10 @@ enum LiftoffBailoutReason : int8_t {
 
 V8_EXPORT_PRIVATE WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&, int func_index,
-    Counters*, WasmFeatures* detected_features, Vector<int> breakpoints = {});
+    Counters*, WasmFeatures* detected_features, Vector<int> breakpoints = {},
+    std::unique_ptr<DebugSideTable>* = nullptr);
 
-V8_EXPORT_PRIVATE DebugSideTable GenerateLiftoffDebugSideTable(
+V8_EXPORT_PRIVATE std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&);
 
 }  // namespace wasm

src/wasm/wasm-code-manager.cc

@@ -1823,8 +1823,10 @@ void NativeModule::FreeCode(Vector<WasmCode* const> codes) {
   // Free the code space.
   code_allocator_.FreeCode(codes);
 
-  // Free the {WasmCode} objects. This will also unregister trap handler data.
   base::MutexGuard guard(&allocation_mutex_);
+  // Remove debug side tables for all removed code objects.
+  if (debug_info_) debug_info_->RemoveDebugSideTables(codes);
+  // Free the {WasmCode} objects. This will also unregister trap handler data.
   for (WasmCode* code : codes) {
     DCHECK_EQ(1, owned_code_.count(code->instruction_start()));
     owned_code_.erase(code->instruction_start());

src/wasm/wasm-debug.cc

@@ -499,10 +499,9 @@ class DebugInfoImpl {
     // Only Liftoff code can be inspected.
     if (!code->is_liftoff()) return local_scope_object;
 
-    const WasmModule* module = native_module_->module();
-    const WasmFunction* function = &module->functions[code->index()];
-    DebugSideTable* debug_side_table =
-        GetDebugSideTable(isolate->allocator(), function->func_index);
+    auto* module = native_module_->module();
+    auto* function = &module->functions[code->index()];
+    auto* debug_side_table = GetDebugSideTable(code, isolate->allocator());
     int pc_offset = static_cast<int>(pc - code->instruction_start());
     auto* debug_side_table_entry = debug_side_table->GetEntry(pc_offset);
     DCHECK_NOT_NULL(debug_side_table_entry);
@@ -597,50 +596,61 @@ class DebugInfoImpl {
     FunctionBody body{function->sig, function->code.offset(),
                       wire_bytes.begin() + function->code.offset(),
                       wire_bytes.begin() + function->code.end_offset()};
+    std::unique_ptr<DebugSideTable> debug_sidetable;
     WasmCompilationResult result = ExecuteLiftoffCompilation(
         native_module_->engine()->allocator(), &env, body, func_index, nullptr,
-        nullptr, VectorOf(breakpoints));
+        nullptr, VectorOf(breakpoints), &debug_sidetable);
     DCHECK(result.succeeded());
+    DCHECK_NOT_NULL(debug_sidetable);
 
     WasmCodeRefScope wasm_code_ref_scope;
     WasmCode* new_code = native_module_->AddCompiledCode(std::move(result));
 
+    bool added =
+        debug_side_tables_.emplace(new_code, std::move(debug_sidetable)).second;
+    DCHECK(added);
+    USE(added);
+
     // TODO(clemensb): OSR active frames on the stack (on all threads).
     USE(new_code);
   }
 
- private:
-  DebugSideTable* GetDebugSideTable(AccountingAllocator* allocator,
-                                    int func_index) {
+  void RemoveDebugSideTables(Vector<WasmCode* const> codes) {
     base::MutexGuard guard(&mutex_);
-    if (debug_side_tables_.empty()) {
-      debug_side_tables_.resize(native_module_->module()->functions.size());
+    for (auto* code : codes) {
+      debug_side_tables_.erase(code);
     }
-    if (auto& existing_table = debug_side_tables_[func_index]) {
+  }
+
+ private:
+  const DebugSideTable* GetDebugSideTable(WasmCode* code,
+                                          AccountingAllocator* allocator) {
+    base::MutexGuard guard(&mutex_);
+    if (auto& existing_table = debug_side_tables_[code]) {
       return existing_table.get();
     }
 
     // Otherwise create the debug side table now.
-    const WasmModule* module = native_module_->module();
-    const WasmFunction* function = &module->functions[func_index];
+    auto* module = native_module_->module();
+    auto* function = &module->functions[code->index()];
     ModuleWireBytes wire_bytes{native_module_->wire_bytes()};
     Vector<const byte> function_bytes = wire_bytes.GetFunctionBytes(function);
     CompilationEnv env = native_module_->CreateCompilationEnv();
     FunctionBody func_body{function->sig, 0, function_bytes.begin(),
                            function_bytes.end()};
-    DebugSideTable debug_side_table =
+    std::unique_ptr<DebugSideTable> debug_side_table =
         GenerateLiftoffDebugSideTable(allocator, &env, func_body);
+    DebugSideTable* ret = debug_side_table.get();
 
     // Install into cache and return.
-    debug_side_tables_[func_index] =
-        std::make_unique<DebugSideTable>(std::move(debug_side_table));
-    return debug_side_tables_[func_index].get();
+    debug_side_tables_[code] = std::move(debug_side_table);
+    return ret;
   }
 
   // Get the value of a local (including parameters) or stack value. Stack
   // values follow the locals in the same index space.
   WasmValue GetValue(const DebugSideTable::Entry* debug_side_table_entry,
-                     ValueType type, int index, Address stack_address) {
+                     ValueType type, int index, Address stack_address) const {
     if (debug_side_table_entry->IsConstant(index)) {
       DCHECK(type == kWasmI32 || type == kWasmI64);
       return type == kWasmI32
@@ -667,10 +677,11 @@ class DebugInfoImpl {
   NativeModule* const native_module_;
 
   // {mutex_} protects all fields below.
-  base::Mutex mutex_;
+  mutable base::Mutex mutex_;
 
-  // DebugSideTable per function, lazily initialized.
-  std::vector<std::unique_ptr<DebugSideTable>> debug_side_tables_;
+  // DebugSideTable per code object, lazily initialized.
+  std::unordered_map<WasmCode*, std::unique_ptr<DebugSideTable>>
+      debug_side_tables_;
 
   // Names of locals, lazily decoded from the wire bytes.
   std::unique_ptr<LocalNames> local_names_;
@@ -700,6 +711,10 @@ void DebugInfo::SetBreakpoint(int func_index, int offset) {
   impl_->SetBreakpoint(func_index, offset);
 }
 
+void DebugInfo::RemoveDebugSideTables(Vector<WasmCode* const> code) {
+  impl_->RemoveDebugSideTables(code);
+}
+
 }  // namespace wasm
 
 namespace {

src/wasm/wasm-debug.h

@@ -21,6 +21,8 @@ namespace internal {
 template <typename T>
 class Handle;
 class JSObject;
+template <typename T>
+class Vector;
 class WasmInstanceObject;
 
 namespace wasm {
@@ -28,6 +30,7 @@ namespace wasm {
 class DebugInfoImpl;
 class LocalNames;
 class NativeModule;
+class WasmCode;
 class WireBytesRef;
 
 // Side table storing information used to inspect Liftoff frames at runtime.
@@ -153,6 +156,8 @@ class DebugInfo {
 
   void SetBreakpoint(int func_index, int offset);
 
+  void RemoveDebugSideTables(Vector<WasmCode* const>);
+
  private:
   std::unique_ptr<DebugInfoImpl> impl_;
 };

test/cctest/wasm/test-liftoff-inspection.cc

@@ -61,7 +61,7 @@ class LiftoffCompileEnvironment {
     CHECK_EQ(detected1, detected2);
   }
 
-  DebugSideTable GenerateDebugSideTable(
+  std::unique_ptr<DebugSideTable> GenerateDebugSideTable(
       std::initializer_list<ValueType> return_types,
       std::initializer_list<ValueType> param_types,
       std::initializer_list<uint8_t> raw_function_bytes) {
@@ -156,20 +156,20 @@ std::ostream& operator<<(std::ostream& out,
 
 void CheckDebugSideTable(std::vector<ValueType> expected_local_types,
                          std::vector<DebugSideTableEntry> expected_entries,
-                         const wasm::DebugSideTable& debug_side_table) {
+                         const wasm::DebugSideTable* debug_side_table) {
   std::vector<ValueType> local_types;
-  for (int i = 0; i < debug_side_table.num_locals(); ++i) {
-    local_types.push_back(debug_side_table.local_type(i));
+  for (int i = 0; i < debug_side_table->num_locals(); ++i) {
+    local_types.push_back(debug_side_table->local_type(i));
   }
   std::vector<DebugSideTableEntry> entries;
-  for (auto& entry : debug_side_table.entries()) {
+  for (auto& entry : debug_side_table->entries()) {
     std::vector<ValueType> stack_types;
     for (int i = 0; i < entry.stack_height(); ++i) {
       stack_types.push_back(entry.stack_type(i));
     }
     std::vector<std::pair<int, int>> constants;
     int locals_plus_stack =
-        debug_side_table.num_locals() + entry.stack_height();
+        debug_side_table->num_locals() + entry.stack_height();
     for (int i = 0; i < locals_plus_stack; ++i) {
       if (entry.IsConstant(i)) constants.emplace_back(i, entry.GetConstant(i));
     }
@@ -228,7 +228,7 @@ TEST(Liftoff_debug_side_table_simple) {
                           // OOL stack check, stack: {}
                           {{}, {}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 TEST(Liftoff_debug_side_table_call) {
@@ -244,7 +244,7 @@ TEST(Liftoff_debug_side_table_call) {
                           // OOL stack check, stack: {}
                           {{}, {}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 TEST(Liftoff_debug_side_table_call_const) {
@@ -262,7 +262,7 @@ TEST(Liftoff_debug_side_table_call_const) {
                           // OOL stack check, stack: {}
                           {{}, {}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 TEST(Liftoff_debug_side_table_indirect_call) {
@@ -283,7 +283,7 @@ TEST(Liftoff_debug_side_table_indirect_call) {
                           // OOL trap (sig mismatch), stack: {kConst}
                           {{kWasmI32}, {{1, kConst}}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 TEST(Liftoff_debug_side_table_loop) {
@@ -299,7 +299,7 @@ TEST(Liftoff_debug_side_table_loop) {
                           // OOL loop stack check, stack: {kConst}
                           {{kWasmI32}, {{1, kConst}}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 TEST(Liftoff_debug_side_table_trap) {
@@ -316,7 +316,7 @@ TEST(Liftoff_debug_side_table_trap) {
                           // OOL trap (result unrepresentable), stack: {}
                           {{}, {}},
                       },
-                      debug_side_table);
+                      debug_side_table.get());
 }
 
 }  // namespace wasm