[wasm] Keep call_indirect index on the stack

When a call_indirect fails because of a signature mismatch or a null
target, the value stack generated for debug doesn't contain the target
index anymore, which makes it hard for users to understand the error.

Keep the index on the stack, and ensure that the index is not modified
until we generate the debug info. Previously, the index was shifted
in-place to compute various offsets. Instead, use scaled loads to
compute the offset directly in the load instruction.

R=clemensb@chromium.org

Bug: chromium:1350384
Change-Id: Iad5359ec80deef25a69ac119119a0b5ca559a336
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3854309Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82780}

src/wasm/baseline/arm/liftoff-assembler-arm.h

@@ -8,6 +8,7 @@
 #include "src/base/platform/wrappers.h"
 #include "src/base/v8-fallthrough.h"
 #include "src/codegen/arm/register-arm.h"
+#include "src/common/globals.h"
 #include "src/heap/memory-chunk.h"
 #include "src/wasm/baseline/liftoff-assembler.h"
 #include "src/wasm/baseline/liftoff-register.h"
@@ -90,11 +91,17 @@ inline MemOperand GetInstanceOperand() { return GetStackSlot(kInstanceOffset); }
 
 inline MemOperand GetMemOp(LiftoffAssembler* assm,
                            UseScratchRegisterScope* temps, Register addr,
-                           Register offset, int32_t offset_imm) {
+                           Register offset, int32_t offset_imm,
+                           unsigned shift_amount = 0) {
   if (offset != no_reg) {
-    if (offset_imm == 0) return MemOperand(addr, offset);
+    if (offset_imm == 0) return MemOperand(addr, offset, LSL, shift_amount);
     Register tmp = temps->Acquire();
-    assm->add(tmp, offset, Operand(offset_imm));
+    if (shift_amount == 0) {
+      assm->add(tmp, offset, Operand(offset_imm));
+    } else {
+      assm->lsl(tmp, offset, Operand(shift_amount));
+      assm->add(tmp, tmp, Operand(offset_imm));
+    }
     return MemOperand(addr, tmp);
   }
   return MemOperand(addr, offset_imm);
@@ -644,12 +651,17 @@ namespace liftoff {
 inline void LoadInternal(LiftoffAssembler* lasm, LiftoffRegister dst,
                          Register src_addr, Register offset_reg,
                          int32_t offset_imm, LoadType type,
-                         uint32_t* protected_load_pc = nullptr) {
+                         uint32_t* protected_load_pc = nullptr,
+                         bool needs_shift = false) {
+  unsigned shift_amount = needs_shift ? type.size_log_2() : 0;
   DCHECK_IMPLIES(type.value_type() == kWasmI64, dst.is_gp_pair());
   UseScratchRegisterScope temps(lasm);
   if (type.value() == LoadType::kF64Load ||
       type.value() == LoadType::kF32Load ||
       type.value() == LoadType::kS128Load) {
+    // Remove the DCHECK and implement scaled offsets for these types if needed.
+    // For now this path is never used.
+    DCHECK(!needs_shift);
     Register actual_src_addr = liftoff::CalculateActualAddress(
         lasm, &temps, src_addr, offset_reg, offset_imm);
     if (type.value() == LoadType::kF64Load) {
@@ -671,8 +683,8 @@ inline void LoadInternal(LiftoffAssembler* lasm, LiftoffRegister dst,
               NeonMemOperand(actual_src_addr));
     }
   } else {
-    MemOperand src_op =
-        liftoff::GetMemOp(lasm, &temps, src_addr, offset_reg, offset_imm);
+    MemOperand src_op = liftoff::GetMemOp(lasm, &temps, src_addr, offset_reg,
+                                          offset_imm, shift_amount);
     if (protected_load_pc) *protected_load_pc = __ pc_offset();
     switch (type.value()) {
       case LoadType::kI32Load8U:
@@ -737,10 +749,10 @@ inline void LoadInternal(LiftoffAssembler* lasm, LiftoffRegister dst,
 
 void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr,
                                          Register offset_reg,
-                                         int32_t offset_imm) {
+                                         int32_t offset_imm, bool needs_shift) {
   static_assert(kTaggedSize == kInt32Size);
   liftoff::LoadInternal(this, LiftoffRegister(dst), src_addr, offset_reg,
-                        offset_imm, LoadType::kI32Load);
+                        offset_imm, LoadType::kI32Load, nullptr, needs_shift);
 }
 
 void LiftoffAssembler::LoadFullPointer(Register dst, Register src_addr,
@@ -793,12 +805,13 @@ void LiftoffAssembler::StoreTaggedPointer(Register dst_addr,
 void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
                             Register offset_reg, uint32_t offset_imm,
                             LoadType type, uint32_t* protected_load_pc,
-                            bool /* is_load_mem */, bool /* i64_offset */) {
+                            bool /* is_load_mem */, bool /* i64_offset */,
+                            bool needs_shift) {
   // Offsets >=2GB are statically OOB on 32-bit systems.
   DCHECK_LE(offset_imm, std::numeric_limits<int32_t>::max());
   liftoff::LoadInternal(this, dst, src_addr, offset_reg,
                         static_cast<int32_t>(offset_imm), type,
-                        protected_load_pc);
+                        protected_load_pc, needs_shift);
 }
 
 void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,

src/wasm/baseline/arm64/liftoff-assembler-arm64.h

@@ -131,15 +131,16 @@ template <typename T>
 inline MemOperand GetMemOp(LiftoffAssembler* assm,
                            UseScratchRegisterScope* temps, Register addr,
                            Register offset, T offset_imm,
-                           bool i64_offset = false) {
+                           bool i64_offset = false, unsigned shift_amount = 0) {
   if (!offset.is_valid()) return MemOperand(addr.X(), offset_imm);
   Register effective_addr = addr.X();
   if (offset_imm) {
     effective_addr = temps->AcquireX();
     assm->Add(effective_addr, addr.X(), offset_imm);
   }
-  return i64_offset ? MemOperand(effective_addr, offset.X())
-                    : MemOperand(effective_addr, offset.W(), UXTW);
+  return i64_offset
+             ? MemOperand(effective_addr, offset.X(), LSL, shift_amount)
+             : MemOperand(effective_addr, offset.W(), UXTW, shift_amount);
 }
 
 // Compute the effective address (sum of |addr|, |offset| (if given) and
@@ -470,10 +471,11 @@ void LiftoffAssembler::ResetOSRTarget() {}
 
 void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr,
                                          Register offset_reg,
-                                         int32_t offset_imm) {
+                                         int32_t offset_imm, bool needs_shift) {
   UseScratchRegisterScope temps(this);
-  MemOperand src_op =
-      liftoff::GetMemOp(this, &temps, src_addr, offset_reg, offset_imm);
+  unsigned shift_amount = !needs_shift ? 0 : COMPRESS_POINTERS_BOOL ? 2 : 3;
+  MemOperand src_op = liftoff::GetMemOp(this, &temps, src_addr, offset_reg,
+                                        offset_imm, false, shift_amount);
   LoadTaggedPointerField(dst, src_op);
 }
 
@@ -527,10 +529,12 @@ void LiftoffAssembler::StoreTaggedPointer(Register dst_addr,
 void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
                             Register offset_reg, uintptr_t offset_imm,
                             LoadType type, uint32_t* protected_load_pc,
-                            bool /* is_load_mem */, bool i64_offset) {
+                            bool /* is_load_mem */, bool i64_offset,
+                            bool needs_shift) {
   UseScratchRegisterScope temps(this);
+  unsigned shift_amount = needs_shift ? type.size_log_2() : 0;
   MemOperand src_op = liftoff::GetMemOp(this, &temps, src_addr, offset_reg,
-                                        offset_imm, i64_offset);
+                                        offset_imm, i64_offset, shift_amount);
   if (protected_load_pc) *protected_load_pc = pc_offset();
   switch (type.value()) {
     case LoadType::kI32Load8U:

src/wasm/baseline/ia32/liftoff-assembler-ia32.h

@@ -385,11 +385,12 @@ void LiftoffAssembler::ResetOSRTarget() {}
 
 void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr,
                                          Register offset_reg,
-                                         int32_t offset_imm) {
+                                         int32_t offset_imm, bool needs_shift) {
   DCHECK_GE(offset_imm, 0);
   static_assert(kTaggedSize == kInt32Size);
   Load(LiftoffRegister(dst), src_addr, offset_reg,
-       static_cast<uint32_t>(offset_imm), LoadType::kI32Load);
+       static_cast<uint32_t>(offset_imm), LoadType::kI32Load, nullptr, false,
+       false, needs_shift);
 }
 
 void LiftoffAssembler::LoadFullPointer(Register dst, Register src_addr,
@@ -434,13 +435,17 @@ void LiftoffAssembler::StoreTaggedPointer(Register dst_addr,
 void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
                             Register offset_reg, uint32_t offset_imm,
                             LoadType type, uint32_t* protected_load_pc,
-                            bool /* is_load_mem */, bool i64_offset) {
+                            bool /* is_load_mem */, bool i64_offset,
+                            bool needs_shift) {
   // Offsets >=2GB are statically OOB on 32-bit systems.
   DCHECK_LE(offset_imm, std::numeric_limits<int32_t>::max());
   DCHECK_EQ(type.value_type() == kWasmI64, dst.is_gp_pair());
-  Operand src_op = offset_reg == no_reg
-                       ? Operand(src_addr, offset_imm)
-                       : Operand(src_addr, offset_reg, times_1, offset_imm);
+  static_assert(times_4 == 2);
+  ScaleFactor scale_factor =
+      !needs_shift ? times_1 : static_cast<ScaleFactor>(type.size_log_2());
+  Operand src_op = offset_reg == no_reg ? Operand(src_addr, offset_imm)
+                                        : Operand(src_addr, offset_reg,
+                                                  scale_factor, offset_imm);
   if (protected_load_pc) *protected_load_pc = pc_offset();
 
   switch (type.value()) {

src/wasm/baseline/liftoff-assembler.h

@@ -774,7 +774,8 @@ class LiftoffAssembler : public TurboAssembler {
   inline void SpillInstance(Register instance);
   inline void ResetOSRTarget();
   inline void LoadTaggedPointer(Register dst, Register src_addr,
-                                Register offset_reg, int32_t offset_imm);
+                                Register offset_reg, int32_t offset_imm,
+                                bool offset_reg_needs_shift = false);
   inline void LoadFullPointer(Register dst, Register src_addr,
                               int32_t offset_imm);
   enum SkipWriteBarrier : bool {
@@ -808,7 +809,8 @@ class LiftoffAssembler : public TurboAssembler {
   inline void Load(LiftoffRegister dst, Register src_addr, Register offset_reg,
                    uintptr_t offset_imm, LoadType type,
                    uint32_t* protected_load_pc = nullptr,
-                   bool is_load_mem = false, bool i64_offset = false);
+                   bool is_load_mem = false, bool i64_offset = false,
+                   bool needs_shift = false);
   inline void Store(Register dst_addr, Register offset_reg,
                     uintptr_t offset_imm, LiftoffRegister src, StoreType type,
                     LiftoffRegList pinned,

src/wasm/baseline/liftoff-compiler.cc

@@ -7133,8 +7133,7 @@ class LiftoffCompiler {
       if (!CheckSupportedType(decoder, ret, "return")) return;
     }
 
-    // Pop the index. We'll modify the register's contents later.
-    Register index = __ PopToModifiableRegister().gp();
+    Register index = __ PeekToRegister(0, {}).gp();
 
     LiftoffRegList pinned{index};
     // Get all temporary registers unconditionally up front.
@@ -7186,10 +7185,9 @@ class LiftoffCompiler {
                   WasmIndirectFunctionTable::kSigIdsOffset),
               kPointerLoadType);
     }
-    // Shift {index} by 2 (multiply by 4) to represent kInt32Size items.
     static_assert((1 << 2) == kInt32Size);
-    __ emit_i32_shli(index, index, 2);
-    __ Load(LiftoffRegister(scratch), table, index, 0, LoadType::kI32Load);
+    __ Load(LiftoffRegister(scratch), table, index, 0, LoadType::kI32Load,
+            nullptr, false, false, true);
 
     // Compare against expected signature.
     if (v8_flags.wasm_type_canonicalization) {
@@ -7207,19 +7205,14 @@ class LiftoffCompiler {
 
     Label* sig_mismatch_label =
         AddOutOfLineTrap(decoder, WasmCode::kThrowWasmTrapFuncSigMismatch);
+    __ DropValues(1);
     {
       FREEZE_STATE(trapping);
       __ emit_cond_jump(kUnequal, sig_mismatch_label, kPointerKind, scratch,
                         tmp_const, trapping);
     }
 
-    // At this point {index} has already been multiplied by 4.
     CODE_COMMENT("Execute indirect call");
-    if (kTaggedSize != kInt32Size) {
-      DCHECK_EQ(kTaggedSize, kInt32Size * 2);
-      // Multiply {index} by another 2 to represent kTaggedSize items.
-      __ emit_i32_add(index, index, index);
-    }
     // At this point {index} has already been multiplied by kTaggedSize.
 
     // Load the instance from {instance->ift_instances[key]}
@@ -7231,14 +7224,8 @@ class LiftoffCompiler {
           wasm::ObjectAccess::ToTagged(WasmIndirectFunctionTable::kRefsOffset));
     }
     __ LoadTaggedPointer(tmp_const, table, index,
-                         ObjectAccess::ElementOffsetInTaggedFixedArray(0));
-
-    if (kTaggedSize != kSystemPointerSize) {
-      DCHECK_EQ(kSystemPointerSize, kTaggedSize * 2);
-      // Multiply {index} by another 2 to represent kSystemPointerSize items.
-      __ emit_i32_add(index, index, index);
-    }
-    // At this point {index} has already been multiplied by kSystemPointerSize.
+                         ObjectAccess::ElementOffsetInTaggedFixedArray(0),
+                         true);
 
     Register* explicit_instance = &tmp_const;
 
@@ -7252,7 +7239,8 @@ class LiftoffCompiler {
                   WasmIndirectFunctionTable::kTargetsOffset),
               kPointerLoadType);
     }
-    __ Load(LiftoffRegister(scratch), table, index, 0, kPointerLoadType);
+    __ Load(LiftoffRegister(scratch), table, index, 0, kPointerLoadType,
+            nullptr, false, false, true);
 
     auto call_descriptor =
         compiler::GetWasmCallDescriptor(compilation_zone_, imm.sig);

src/wasm/baseline/x64/liftoff-assembler-x64.h

@@ -81,18 +81,19 @@ constexpr Operand kInstanceOperand = GetStackSlot(kInstanceOffset);
 constexpr Operand kOSRTargetSlot = GetStackSlot(kOSRTargetOffset);
 
 inline Operand GetMemOp(LiftoffAssembler* assm, Register addr,
-                        Register offset_reg, uintptr_t offset_imm) {
+                        Register offset_reg, uintptr_t offset_imm,
+                        ScaleFactor scale_factor = times_1) {
   if (is_uint31(offset_imm)) {
     int32_t offset_imm32 = static_cast<int32_t>(offset_imm);
     return offset_reg == no_reg
                ? Operand(addr, offset_imm32)
-               : Operand(addr, offset_reg, times_1, offset_imm32);
+               : Operand(addr, offset_reg, scale_factor, offset_imm32);
   }
   // Offset immediate does not fit in 31 bits.
   Register scratch = kScratchRegister;
   assm->TurboAssembler::Move(scratch, offset_imm);
   if (offset_reg != no_reg) assm->addq(scratch, offset_reg);
-  return Operand(addr, scratch, times_1, 0);
+  return Operand(addr, scratch, scale_factor, 0);
 }
 
 inline void Load(LiftoffAssembler* assm, LiftoffRegister dst, Operand src,
@@ -381,11 +382,15 @@ void LiftoffAssembler::ResetOSRTarget() {
 
 void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr,
                                          Register offset_reg,
-                                         int32_t offset_imm) {
+                                         int32_t offset_imm, bool needs_shift) {
   DCHECK_GE(offset_imm, 0);
   if (offset_reg != no_reg) AssertZeroExtended(offset_reg);
-  Operand src_op = liftoff::GetMemOp(this, src_addr, offset_reg,
-                                     static_cast<uint32_t>(offset_imm));
+  ScaleFactor scale_factor = !needs_shift             ? times_1
+                             : COMPRESS_POINTERS_BOOL ? times_4
+                                                      : times_8;
+  Operand src_op =
+      liftoff::GetMemOp(this, src_addr, offset_reg,
+                        static_cast<uint32_t>(offset_imm), scale_factor);
   LoadTaggedPointerField(dst, src_op);
 }
 
@@ -440,9 +445,14 @@ void LiftoffAssembler::AtomicLoad(LiftoffRegister dst, Register src_addr,
 void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
                             Register offset_reg, uintptr_t offset_imm,
                             LoadType type, uint32_t* protected_load_pc,
-                            bool /* is_load_mem */, bool i64_offset) {
+                            bool /* is_load_mem */, bool i64_offset,
+                            bool needs_shift) {
   if (offset_reg != no_reg && !i64_offset) AssertZeroExtended(offset_reg);
-  Operand src_op = liftoff::GetMemOp(this, src_addr, offset_reg, offset_imm);
+  static_assert(times_4 == 2);
+  ScaleFactor scale_factor =
+      needs_shift ? static_cast<ScaleFactor>(type.size_log_2()) : times_1;
+  Operand src_op =
+      liftoff::GetMemOp(this, src_addr, offset_reg, offset_imm, scale_factor);
   if (protected_load_pc) *protected_load_pc = pc_offset();
   switch (type.value()) {
     case LoadType::kI32Load8U:

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

@@ -355,8 +355,9 @@ TEST(Liftoff_debug_side_table_indirect_call) {
   constexpr int kConst = 47;
   auto debug_side_table = env.GenerateDebugSideTable(
       {kWasmI32}, {kWasmI32},
-      {WASM_I32_ADD(WASM_CALL_INDIRECT(0, WASM_I32V_1(47), WASM_LOCAL_GET(0)),
-                    WASM_LOCAL_GET(0))});
+      {WASM_I32_ADD(
+          WASM_CALL_INDIRECT(0, WASM_I32V_1(kConst), WASM_LOCAL_GET(0)),
+          WASM_LOCAL_GET(0))});
   CheckDebugSideTable(
       {
           // function entry, local in register.
@@ -365,10 +366,11 @@ TEST(Liftoff_debug_side_table_indirect_call) {
           {1, {Stack(0, kWasmI32)}},
           // OOL stack check, local still spilled.
           {1, {}},
-          // OOL trap (invalid index), local still spilled, stack has {kConst}.
-          {2, {Constant(1, kWasmI32, kConst)}},
+          // OOL trap (invalid index), local still spilled, stack has {kConst,
+          // kStack}.
+          {3, {Constant(1, kWasmI32, kConst), Stack(2, kWasmI32)}},
           // OOL trap (sig mismatch), stack unmodified.
-          {2, {}},
+          {3, {}},
       },
       debug_side_table.get());
 }