[arm64][x64][liftoff] Fix trap handling on load lane

This is a reland of 1786f8d7. It turned
out that also x64 is broken, and only for TurboFan. Both is fixed now.

Original change's description:
> [arm64][liftoff] Fix trap handling on load lane
>
> This fixes the registered {protected_load_pc} to (always) point to the
> actual load instruction. If {dst != src} we would emit a register move
> before the load, and the trap handler would then not recognize the PC
> where the signal occurs, leading to a segfault.
>
> R=thibaudm@chromium.org
>
> Bug: chromium:1242300, v8:12018
> Change-Id: I3ed2a8307e353fd85a7ddedf6ecb73e90a112d32
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3136454
> Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#76642}

Bug: chromium:1242300, v8:12018
Change-Id: I79284ab9815f5363f759569d98c8c4b52d48e738
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3140609Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76698}

src/codegen/x64/macro-assembler-x64.cc

@@ -2024,16 +2024,17 @@ using NoAvxFn = void (Assembler::*)(XMMRegister, Src, uint8_t);
 template <typename Src>
 void PinsrHelper(Assembler* assm, AvxFn<Src> avx, NoAvxFn<Src> noavx,
                  XMMRegister dst, XMMRegister src1, Src src2, uint8_t imm8,
+                 uint32_t* load_pc_offset = nullptr,
                  base::Optional<CpuFeature> feature = base::nullopt) {
   if (CpuFeatures::IsSupported(AVX)) {
     CpuFeatureScope scope(assm, AVX);
+    if (load_pc_offset) *load_pc_offset = assm->pc_offset();
     (assm->*avx)(dst, src1, src2, imm8);
     return;
   }
 
-  if (dst != src1) {
-    assm->movaps(dst, src1);
-  }
+  if (dst != src1) assm->movaps(dst, src1);
+  if (load_pc_offset) *load_pc_offset = assm->pc_offset();
   if (feature.has_value()) {
     DCHECK(CpuFeatures::IsSupported(*feature));
     CpuFeatureScope scope(assm, *feature);
@@ -2045,40 +2046,41 @@ void PinsrHelper(Assembler* assm, AvxFn<Src> avx, NoAvxFn<Src> noavx,
 }  // namespace
 
 void TurboAssembler::Pinsrb(XMMRegister dst, XMMRegister src1, Register src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrb, &Assembler::pinsrb, dst, src1, src2,
-              imm8, base::Optional<CpuFeature>(SSE4_1));
+              imm8, load_pc_offset, {SSE4_1});
 }
 
 void TurboAssembler::Pinsrb(XMMRegister dst, XMMRegister src1, Operand src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrb, &Assembler::pinsrb, dst, src1, src2,
-              imm8, base::Optional<CpuFeature>(SSE4_1));
+              imm8, load_pc_offset, {SSE4_1});
 }
 
 void TurboAssembler::Pinsrw(XMMRegister dst, XMMRegister src1, Register src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrw, &Assembler::pinsrw, dst, src1, src2,
-              imm8);
+              imm8, load_pc_offset);
 }
 
 void TurboAssembler::Pinsrw(XMMRegister dst, XMMRegister src1, Operand src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrw, &Assembler::pinsrw, dst, src1, src2,
-              imm8);
+              imm8, load_pc_offset);
 }
 
 void TurboAssembler::Pinsrd(XMMRegister dst, XMMRegister src1, Register src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   // Need a fall back when SSE4_1 is unavailable. Pinsrb and Pinsrq are used
   // only by Wasm SIMD, which requires SSE4_1 already.
   if (CpuFeatures::IsSupported(SSE4_1)) {
     PinsrHelper(this, &Assembler::vpinsrd, &Assembler::pinsrd, dst, src1, src2,
-                imm8, base::Optional<CpuFeature>(SSE4_1));
+                imm8, load_pc_offset, {SSE4_1});
     return;
   }
 
   Movd(kScratchDoubleReg, src2);
+  if (load_pc_offset) *load_pc_offset = pc_offset();
   if (imm8 == 1) {
     punpckldq(dst, kScratchDoubleReg);
   } else {
@@ -2088,16 +2090,17 @@ void TurboAssembler::Pinsrd(XMMRegister dst, XMMRegister src1, Register src2,
 }
 
 void TurboAssembler::Pinsrd(XMMRegister dst, XMMRegister src1, Operand src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   // Need a fall back when SSE4_1 is unavailable. Pinsrb and Pinsrq are used
   // only by Wasm SIMD, which requires SSE4_1 already.
   if (CpuFeatures::IsSupported(SSE4_1)) {
     PinsrHelper(this, &Assembler::vpinsrd, &Assembler::pinsrd, dst, src1, src2,
-                imm8, base::Optional<CpuFeature>(SSE4_1));
+                imm8, load_pc_offset, {SSE4_1});
     return;
   }
 
   Movd(kScratchDoubleReg, src2);
+  if (load_pc_offset) *load_pc_offset = pc_offset();
   if (imm8 == 1) {
     punpckldq(dst, kScratchDoubleReg);
   } else {
@@ -2106,24 +2109,26 @@ void TurboAssembler::Pinsrd(XMMRegister dst, XMMRegister src1, Operand src2,
   }
 }
 
-void TurboAssembler::Pinsrd(XMMRegister dst, Register src2, uint8_t imm8) {
-  Pinsrd(dst, dst, src2, imm8);
+void TurboAssembler::Pinsrd(XMMRegister dst, Register src2, uint8_t imm8,
+                            uint32_t* load_pc_offset) {
+  Pinsrd(dst, dst, src2, imm8, load_pc_offset);
 }
 
-void TurboAssembler::Pinsrd(XMMRegister dst, Operand src2, uint8_t imm8) {
-  Pinsrd(dst, dst, src2, imm8);
+void TurboAssembler::Pinsrd(XMMRegister dst, Operand src2, uint8_t imm8,
+                            uint32_t* load_pc_offset) {
+  Pinsrd(dst, dst, src2, imm8, load_pc_offset);
 }
 
 void TurboAssembler::Pinsrq(XMMRegister dst, XMMRegister src1, Register src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrq, &Assembler::pinsrq, dst, src1, src2,
-              imm8, base::Optional<CpuFeature>(SSE4_1));
+              imm8, load_pc_offset, {SSE4_1});
 }
 
 void TurboAssembler::Pinsrq(XMMRegister dst, XMMRegister src1, Operand src2,
-                            uint8_t imm8) {
+                            uint8_t imm8, uint32_t* load_pc_offset) {
   PinsrHelper(this, &Assembler::vpinsrq, &Assembler::pinsrq, dst, src1, src2,
-              imm8, base::Optional<CpuFeature>(SSE4_1));
+              imm8, load_pc_offset, {SSE4_1});
 }
 
 void TurboAssembler::Absps(XMMRegister dst, XMMRegister src) {

src/codegen/x64/macro-assembler-x64.h

@@ -418,16 +418,26 @@ class V8_EXPORT_PRIVATE TurboAssembler
   // Non-SSE2 instructions.
   void Pextrd(Register dst, XMMRegister src, uint8_t imm8);
 
-  void Pinsrb(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8);
-  void Pinsrb(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8);
-  void Pinsrw(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8);
-  void Pinsrw(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8);
-  void Pinsrd(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8);
-  void Pinsrd(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8);
-  void Pinsrd(XMMRegister dst, Register src2, uint8_t imm8);
-  void Pinsrd(XMMRegister dst, Operand src2, uint8_t imm8);
-  void Pinsrq(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8);
-  void Pinsrq(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8);
+  void Pinsrb(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrb(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrw(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrw(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrd(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrd(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrd(XMMRegister dst, Register src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrd(XMMRegister dst, Operand src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrq(XMMRegister dst, XMMRegister src1, Register src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
+  void Pinsrq(XMMRegister dst, XMMRegister src1, Operand src2, uint8_t imm8,
+              uint32_t* load_pc_offset = nullptr);
 
   void Absps(XMMRegister dst, XMMRegister src);
   void Negps(XMMRegister dst, XMMRegister src);

src/compiler/backend/x64/code-generator-x64.cc

@@ -1000,24 +1000,23 @@ void EmitOOLTrapIfNeeded(Zone* zone, CodeGenerator* codegen,
     }                                                                    \
   } while (false)
 
-#define ASSEMBLE_PINSR(ASM_INSTR)                                     \
-  do {                                                                \
-    EmitOOLTrapIfNeeded(zone(), this, opcode, instr, __ pc_offset()); \
-    XMMRegister dst = i.OutputSimd128Register();                      \
-    XMMRegister src = i.InputSimd128Register(0);                      \
-    uint8_t laneidx = i.InputUint8(1);                                \
-    if (HasAddressingMode(instr)) {                                   \
-      __ ASM_INSTR(dst, src, i.MemoryOperand(2), laneidx);            \
-      break;                                                          \
-    }                                                                 \
-    if (instr->InputAt(2)->IsFPRegister()) {                          \
-      __ Movq(kScratchRegister, i.InputDoubleRegister(2));            \
-      __ ASM_INSTR(dst, src, kScratchRegister, laneidx);              \
-    } else if (instr->InputAt(2)->IsRegister()) {                     \
-      __ ASM_INSTR(dst, src, i.InputRegister(2), laneidx);            \
-    } else {                                                          \
-      __ ASM_INSTR(dst, src, i.InputOperand(2), laneidx);             \
-    }                                                                 \
+#define ASSEMBLE_PINSR(ASM_INSTR)                                        \
+  do {                                                                   \
+    XMMRegister dst = i.OutputSimd128Register();                         \
+    XMMRegister src = i.InputSimd128Register(0);                         \
+    uint8_t laneidx = i.InputUint8(1);                                   \
+    uint32_t load_offset;                                                \
+    if (HasAddressingMode(instr)) {                                      \
+      __ ASM_INSTR(dst, src, i.MemoryOperand(2), laneidx, &load_offset); \
+    } else if (instr->InputAt(2)->IsFPRegister()) {                      \
+      __ Movq(kScratchRegister, i.InputDoubleRegister(2));               \
+      __ ASM_INSTR(dst, src, kScratchRegister, laneidx, &load_offset);   \
+    } else if (instr->InputAt(2)->IsRegister()) {                        \
+      __ ASM_INSTR(dst, src, i.InputRegister(2), laneidx, &load_offset); \
+    } else {                                                             \
+      __ ASM_INSTR(dst, src, i.InputOperand(2), laneidx, &load_offset);  \
+    }                                                                    \
+    EmitOOLTrapIfNeeded(zone(), this, opcode, instr, load_offset);       \
   } while (false)
 
 #define ASSEMBLE_SEQ_CST_STORE(rep)                                       \

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

@@ -1707,13 +1707,13 @@ void LiftoffAssembler::LoadLane(LiftoffRegister dst, LiftoffRegister src,
   UseScratchRegisterScope temps(this);
   MemOperand src_op{
       liftoff::GetEffectiveAddress(this, &temps, addr, offset_reg, offset_imm)};
-  *protected_load_pc = pc_offset();
 
   MachineType mem_type = type.mem_type();
   if (dst != src) {
     Mov(dst.fp().Q(), src.fp().Q());
   }
 
+  *protected_load_pc = pc_offset();
   if (mem_type == MachineType::Int8()) {
     ld1(dst.fp().B(), laneidx, src_op);
   } else if (mem_type == MachineType::Int16()) {

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

@@ -2407,18 +2407,17 @@ void LiftoffAssembler::LoadLane(LiftoffRegister dst, LiftoffRegister src,
                                 uintptr_t offset_imm, LoadType type,
                                 uint8_t laneidx, uint32_t* protected_load_pc) {
   Operand src_op = liftoff::GetMemOp(this, addr, offset_reg, offset_imm);
-  *protected_load_pc = pc_offset();
 
   MachineType mem_type = type.mem_type();
   if (mem_type == MachineType::Int8()) {
-    Pinsrb(dst.fp(), src.fp(), src_op, laneidx);
+    Pinsrb(dst.fp(), src.fp(), src_op, laneidx, protected_load_pc);
   } else if (mem_type == MachineType::Int16()) {
-    Pinsrw(dst.fp(), src.fp(), src_op, laneidx);
+    Pinsrw(dst.fp(), src.fp(), src_op, laneidx, protected_load_pc);
   } else if (mem_type == MachineType::Int32()) {
-    Pinsrd(dst.fp(), src.fp(), src_op, laneidx);
+    Pinsrd(dst.fp(), src.fp(), src_op, laneidx, protected_load_pc);
   } else {
     DCHECK_EQ(MachineType::Int64(), mem_type);
-    Pinsrq(dst.fp(), src.fp(), src_op, laneidx);
+    Pinsrq(dst.fp(), src.fp(), src_op, laneidx, protected_load_pc);
   }
 }
 

test/mjsunit/mjsunit.status

@@ -1478,8 +1478,9 @@
 
 ##############################################################################
 ['variant == instruction_scheduling or variant == stress_instruction_scheduling', {
-  # BUG(12018): This test currently fails with --turbo-instruction-scheduling.
+  # BUG(12018): These tests currently fail with --turbo-instruction-scheduling.
   'regress/wasm/regress-1231950': [SKIP],
+  'regress/wasm/regress-1242300': [SKIP],
 }],  # variant == instruction_scheduling or variant == stress_instruction_scheduling
 
 ################################################################################

test/mjsunit/regress/wasm/regress-1242300.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.
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+builder.addMemory(16, 32);
+builder.addFunction(undefined, kSig_i_iii)
+  .addBody([
+    kExprI32Const, 0x7f,  // i32.const
+    kExprI32Const, 0x1e,  // i32.const
+    kSimdPrefix, kExprI8x16Splat,  // i8x16.splat
+    kExprI32Const, 0,  // i32.const
+    kSimdPrefix, kExprI8x16Splat,  // i8x16.splat
+    kExprI32Const, 0,  // i32.const
+    kSimdPrefix, kExprI8x16Splat,  // i8x16.splat
+    kSimdPrefix, kExprS128Select,  // s128.select
+    kSimdPrefix, kExprS128Load32Lane, 0x00, 0x89, 0xfe, 0x03, 0x00,  // s128.load32_lane
+    kExprUnreachable,
+]);
+builder.addExport('main', 0);
+const instance = builder.instantiate();
+assertTraps(kTrapMemOutOfBounds, () => instance.exports.main(1, 2, 3));