Reland "[wasm] Emit safepoint info for callee-saved registers in the deopt-index"

The original CL was reverted because PC authentication was missing for
the `caller_pc` in the stack walk. This caused a crash on the CFI bot.

PS1 is the original CL, later patch sets contain the fix.

Original Message:

[wasm] Emit safepoint info for callee-saved registers in the deopt-index

Encode safepoint info of callee-saved registers in the deopt index of
the normal safepoint.

R=clemensb@chromium.org, jkummerow@chromium.org

Change-Id: I633cd715eccc697e888cd381e3bda1a47d0d0851
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2759520Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73464}

src/codegen/safepoint-table.cc

@@ -97,7 +97,7 @@ Safepoint SafepointTableBuilder::DefineSafepoint(Assembler* assembler) {
   deoptimization_info_.push_back(
       DeoptimizationInfo(zone_, assembler->pc_offset_for_safepoint()));
   DeoptimizationInfo& new_info = deoptimization_info_.back();
-  return Safepoint(new_info.indexes);
+  return Safepoint(new_info.stack_indexes, &new_info.register_indexes);
 }
 
 unsigned SafepointTableBuilder::GetCodeOffset() const {
@@ -148,14 +148,22 @@ void SafepointTableBuilder::Emit(Assembler* assembler, int bits_per_entry) {
   STATIC_ASSERT(SafepointTable::kFixedEntrySize == 3 * kIntSize);
   for (const DeoptimizationInfo& info : deoptimization_info_) {
     assembler->dd(info.pc);
-    assembler->dd(info.deopt_index);
+    if (info.register_indexes) {
+      // We emit the register indexes in the same bits as the deopt_index.
+      // Register indexes and deopt_index should not exist at the same time.
+      DCHECK_EQ(info.deopt_index,
+                static_cast<uint32_t>(Safepoint::kNoDeoptimizationIndex));
+      assembler->dd(info.register_indexes);
+    } else {
+      assembler->dd(info.deopt_index);
+    }
     assembler->dd(info.trampoline);
   }
 
   // Emit table of bitmaps.
   ZoneVector<uint8_t> bits(bytes_per_entry, 0, zone_);
   for (const DeoptimizationInfo& info : deoptimization_info_) {
-    ZoneChunkList<int>* indexes = info.indexes;
+    ZoneChunkList<int>* indexes = info.stack_indexes;
     std::fill(bits.begin(), bits.end(), 0);
 
     // Run through the indexes and build a bitmap.
@@ -199,13 +207,15 @@ bool SafepointTableBuilder::IsIdenticalExceptForPc(
     const DeoptimizationInfo& info1, const DeoptimizationInfo& info2) const {
   if (info1.deopt_index != info2.deopt_index) return false;
 
-  ZoneChunkList<int>* indexes1 = info1.indexes;
-  ZoneChunkList<int>* indexes2 = info2.indexes;
+  ZoneChunkList<int>* indexes1 = info1.stack_indexes;
+  ZoneChunkList<int>* indexes2 = info2.stack_indexes;
   if (indexes1->size() != indexes2->size()) return false;
   if (!std::equal(indexes1->begin(), indexes1->end(), indexes2->begin())) {
     return false;
   }
 
+  if (info1.register_indexes != info2.register_indexes) return false;
+
   return true;
 }
 

src/codegen/safepoint-table.h

@@ -50,6 +50,18 @@ class SafepointEntry {
     return deopt_index_;
   }
 
+  uint32_t register_bits() const {
+    // The register bits use the same field as the deopt_index_.
+    DCHECK(is_valid());
+    return deopt_index_;
+  }
+
+  bool has_register_bits() const {
+    // The register bits use the same field as the deopt_index_.
+    DCHECK(is_valid());
+    return deopt_index_ != kNoDeoptIndex;
+  }
+
   bool has_deoptimization_index() const {
     DCHECK(is_valid());
     return deopt_index_ != kNoDeoptIndex;
@@ -61,7 +73,7 @@ class SafepointEntry {
   }
 
  private:
-  unsigned deopt_index_;
+  uint32_t deopt_index_;
   uint8_t* bits_;
   // It needs to be an integer as it is -1 for eager deoptimizations.
   int trampoline_pc_;
@@ -172,11 +184,20 @@ class Safepoint {
  public:
   static const int kNoDeoptimizationIndex = SafepointEntry::kNoDeoptIndex;
 
-  void DefinePointerSlot(int index) { indexes_->push_back(index); }
+  void DefinePointerSlot(int index) { stack_indexes_->push_back(index); }
+
+  void DefineRegister(int reg_code) {
+    // Make sure the recorded index is always less than 31, so that we don't
+    // generate {kNoDeoptimizationIndex} by accident.
+    DCHECK_LT(reg_code, 31);
+    *register_indexes_ |= 1u << reg_code;
+  }
 
  private:
-  explicit Safepoint(ZoneChunkList<int>* indexes) : indexes_(indexes) {}
-  ZoneChunkList<int>* const indexes_;
+  Safepoint(ZoneChunkList<int>* stack_indexes, uint32_t* register_indexes)
+      : stack_indexes_(stack_indexes), register_indexes_(register_indexes) {}
+  ZoneChunkList<int>* const stack_indexes_;
+  uint32_t* register_indexes_;
 
   friend class SafepointTableBuilder;
 };
@@ -213,13 +234,15 @@ class SafepointTableBuilder {
     unsigned pc;
     unsigned deopt_index;
     int trampoline;
-    ZoneChunkList<int>* indexes;
+    ZoneChunkList<int>* stack_indexes;
+    uint32_t register_indexes;
     DeoptimizationInfo(Zone* zone, unsigned pc)
         : pc(pc),
           deopt_index(Safepoint::kNoDeoptimizationIndex),
           trampoline(-1),
-          indexes(zone->New<ZoneChunkList<int>>(
-              zone, ZoneChunkList<int>::StartMode::kSmall)) {}
+          stack_indexes(zone->New<ZoneChunkList<int>>(
+              zone, ZoneChunkList<int>::StartMode::kSmall)),
+          register_indexes(0) {}
   };
 
   // Compares all fields of a {DeoptimizationInfo} except {pc} and {trampoline}.

src/execution/frames-inl.h

@@ -163,7 +163,7 @@ inline Address CommonFrame::caller_fp() const {
 }
 
 inline Address CommonFrame::caller_pc() const {
-  return base::Memory<Address>(ComputePCAddress(fp()));
+  return ReadPC(reinterpret_cast<Address*>(ComputePCAddress(fp())));
 }
 
 inline Address CommonFrame::ComputePCAddress(Address fp) {

src/execution/frames.cc

@@ -1973,8 +1973,24 @@ int WasmFrame::LookupExceptionHandlerInTable() {
 }
 
 void WasmDebugBreakFrame::Iterate(RootVisitor* v) const {
-  // Nothing to iterate here. This will change once we support references in
-  // Liftoff.
+  DCHECK(caller_pc());
+  wasm::WasmCode* code =
+      isolate()->wasm_engine()->code_manager()->LookupCode(caller_pc());
+  DCHECK(code);
+  SafepointTable table(code);
+  SafepointEntry safepoint_entry = table.FindEntry(caller_pc());
+  if (!safepoint_entry.has_register_bits()) return;
+  uint32_t register_bits = safepoint_entry.register_bits();
+
+  while (register_bits != 0) {
+    int reg_code = base::bits::CountTrailingZeros(register_bits);
+    register_bits &= ~(1 << reg_code);
+    FullObjectSlot spill_slot(&Memory<Address>(
+        fp() +
+        WasmDebugBreakFrameConstants::GetPushedGpRegisterOffset(reg_code)));
+
+    v->VisitRootPointer(Root::kTop, nullptr, spill_slot);
+  }
 }
 
 void WasmDebugBreakFrame::Print(StringStream* accumulator, PrintMode mode,

src/wasm/baseline/liftoff-assembler.cc

@@ -537,14 +537,26 @@ void LiftoffAssembler::CacheState::GetTaggedSlotsForOOLCode(
 
 void LiftoffAssembler::CacheState::DefineSafepoint(Safepoint& safepoint) {
   for (const auto& slot : stack_state) {
-    DCHECK(!slot.is_reg());
-
     if (is_reference(slot.kind())) {
+      DCHECK(slot.is_stack());
       safepoint.DefinePointerSlot(GetSafepointIndexForStackSlot(slot));
     }
   }
 }
 
+void LiftoffAssembler::CacheState::DefineSafepointWithCalleeSavedRegisters(
+    Safepoint& safepoint) {
+  for (const auto& slot : stack_state) {
+    if (!is_reference(slot.kind())) continue;
+    if (slot.is_stack()) {
+      safepoint.DefinePointerSlot(GetSafepointIndexForStackSlot(slot));
+    } else {
+      DCHECK(slot.is_reg());
+      safepoint.DefineRegister(slot.reg().gp().code());
+    }
+  }
+}
+
 int LiftoffAssembler::GetTotalFrameSlotCountForGC() const {
   // The GC does not care about the actual number of spill slots, just about
   // the number of references that could be there in the spilling area. Note

src/wasm/baseline/liftoff-assembler.h

@@ -193,6 +193,8 @@ class LiftoffAssembler : public TurboAssembler {
 
     void DefineSafepoint(Safepoint& safepoint);
 
+    void DefineSafepointWithCalleeSavedRegisters(Safepoint& safepoint);
+
     base::SmallVector<VarState, 8> stack_state;
     LiftoffRegList used_registers;
     uint32_t register_use_count[kAfterMaxLiftoffRegCode] = {0};

src/wasm/baseline/liftoff-compiler.cc

@@ -1028,8 +1028,7 @@ class LiftoffCompiler {
     source_position_table_builder_.AddPosition(
         __ pc_offset(), SourcePosition(decoder->position()), true);
     __ CallRuntimeStub(WasmCode::kWasmDebugBreak);
-    // TODO(ahaas): Define a proper safepoint here.
-    safepoint_table_builder_.DefineSafepoint(&asm_);
+    DefineSafepointWithCalleeSavedRegisters();
     RegisterDebugSideTableEntry(decoder,
                                 DebugSideTableBuilder::kAllowRegisters);
   }
@@ -6020,6 +6019,11 @@ class LiftoffCompiler {
     __ cache_state()->DefineSafepoint(safepoint);
   }
 
+  void DefineSafepointWithCalleeSavedRegisters() {
+    Safepoint safepoint = safepoint_table_builder_.DefineSafepoint(&asm_);
+    __ cache_state()->DefineSafepointWithCalleeSavedRegisters(safepoint);
+  }
+
   Register LoadInstanceIntoRegister(LiftoffRegList pinned, Register fallback) {
     Register instance = __ cache_state()->cached_instance;
     if (instance == no_reg) {

src/wasm/wasm-code-manager.cc

@@ -421,8 +421,13 @@ void WasmCode::Disassemble(const char* name, std::ostream& os,
       if (entry.trampoline_pc() != SafepointEntry::kNoTrampolinePC) {
         os << " trampoline: " << std::hex << entry.trampoline_pc() << std::dec;
       }
-      if (entry.has_deoptimization_index()) {
-        os << " deopt: " << std::setw(6) << entry.deoptimization_index();
+      if (entry.has_register_bits()) {
+        os << " registers: ";
+        uint32_t register_bits = entry.register_bits();
+        int bits = 32 - base::bits::CountLeadingZeros32(register_bits);
+        for (int i = bits - 1; i >= 0; --i) {
+          os << ((register_bits >> i) & 1);
+        }
       }
       os << "\n";
     }

test/inspector/debugger/wasm-gc-in-debug-break-expected.txt

+Tests GC within DebugBreak
+
+Running test: test
+Script wasm://wasm/38e28046 byte offset 51: Wasm opcode 0x20 (kExprLocalGet)
+GC triggered
+Debugger.resume
+Hello World (v8://test/instantiate:11:36)
+    at bar (v8://test/instantiate:11:36)
+exports.main returned!
+

test/inspector/debugger/wasm-gc-in-debug-break.js

+// Copyright 2021 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.
+
+// Flags: --experimental-wasm-reftypes --expose-gc
+utils.load('test/inspector/wasm-inspector-test.js');
+
+let {session, contextGroup, Protocol} =
+    InspectorTest.start('Tests GC within DebugBreak');
+session.setupScriptMap();
+
+let builder = new WasmModuleBuilder();
+
+let f_index = builder.addImport('foo', 'bar', kSig_v_r);
+
+builder.addFunction('wasm_A', kSig_v_r)
+    .addBody([
+      kExprLocalGet, 0,           // -
+      kExprCallFunction, f_index  // -
+    ])
+    .exportAs('main');
+
+let module_bytes = builder.toArray();
+
+Protocol.Debugger.onPaused(async message => {
+  let frames = message.params.callFrames;
+  await session.logSourceLocation(frames[0].location);
+  await Protocol.Runtime.evaluate({expression: 'gc()'});
+  InspectorTest.log('GC triggered');
+  let action = 'resume';
+  InspectorTest.log('Debugger.' + action);
+  await Protocol.Debugger[action]();
+})
+
+contextGroup.addScript(`
+function test() {
+  debug(instance.exports.main);
+  instance.exports.main({val: "Hello World"});
+}
+//# sourceURL=test.js`);
+
+InspectorTest.runAsyncTestSuite([async function test() {
+  utils.setLogConsoleApiMessageCalls(true);
+  await Protocol.Debugger.enable();
+  await WasmInspectorTest.instantiate(
+      module_bytes, 'instance', '{foo: {bar: (x) => console.log(x.val)}}');
+  await Protocol.Runtime.evaluate(
+      {expression: 'test()', includeCommandLineAPI: true});
+  InspectorTest.log('exports.main returned!');
+}]);

test/inspector/wasm-inspector-test.js

@@ -36,9 +36,10 @@ WasmInspectorTest.compile = async function(bytes, module_name = 'module') {
 };
 
 WasmInspectorTest.instantiate =
-    async function(bytes, instance_name = 'instance') {
+    async function(bytes, instance_name = 'instance', imports) {
   const instantiate_code = `var ${instance_name} = (${
-      WasmInspectorTest.instantiateFromBuffer})(${JSON.stringify(bytes)});`;
+      WasmInspectorTest.instantiateFromBuffer})(${JSON.stringify(bytes)},
+        ${imports});`;
   await WasmInspectorTest.evalWithUrl(instantiate_code, 'instantiate');
 };