-
Notifications
You must be signed in to change notification settings - Fork 334
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Cherry-pick arm64 loadtaggedpointer fix (#1921)
- Loading branch information
1 parent
b82451d
commit b897dda
Showing
17 changed files
with
234 additions
and
15 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0001-Allow-manually-setting-ValueDeserializer-format-vers.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0002-Allow-manually-setting-ValueSerializer-format-versio.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0006-Make-v8-Locker-automatically-call-isolate-Enter.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0007-Add-an-API-to-capture-and-restore-the-cage-base-poin.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0008-Speed-up-V8-bazel-build-by-always-using-target-cfg.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0011-Randomize-the-initial-ExecutionContextId-used-by-the.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0012-Always-enable-continuation-preserved-data-in-the-bui.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0013-Attach-continuation-context-to-Promise-thenable-task.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
patches/v8/0015-Add-ValueSerializer-SetTreatFunctionsAsHostObjects.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
218 changes: 218 additions & 0 deletions
218
patches/v8/0016-wasm-liftoff-arm64-Fix-LoadTaggedPointer.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,218 @@ | ||
From 8a89d470ac20ef90687c1267ae2932059dc82fa2 Mon Sep 17 00:00:00 2001 | ||
From: Jakob Kummerow <jkummerow@chromium.org> | ||
Date: Mon, 19 Feb 2024 19:59:06 +0100 | ||
Subject: [wasm][liftoff][arm64] Fix LoadTaggedPointer | ||
|
||
The previous way to compute the protected_load_pc didn't account for | ||
the possibility of a constant pool getting emitted. | ||
|
||
Fixed: 325359458 | ||
Change-Id: I7c284175d3c0e9d802ad307adf8d93e721d35361 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5307536 | ||
Reviewed-by: Manos Koukoutos <manoskouk@chromium.org> | ||
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org> | ||
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org> | ||
Cr-Commit-Position: refs/heads/main@{#92412} | ||
|
||
diff --git a/src/wasm/baseline/arm64/liftoff-assembler-arm64-inl.h b/src/wasm/baseline/arm64/liftoff-assembler-arm64-inl.h | ||
index b5eb6dc9b23e3f478425dfe75085cc447cb7cd4e..0be1ffb97778cf0aeb7ce20478ea26c3155ea628 100644 | ||
--- a/src/wasm/baseline/arm64/liftoff-assembler-arm64-inl.h | ||
+++ b/src/wasm/baseline/arm64/liftoff-assembler-arm64-inl.h | ||
@@ -544,6 +544,64 @@ void LiftoffAssembler::SpillInstanceData(Register instance) { | ||
|
||
void LiftoffAssembler::ResetOSRTarget() {} | ||
|
||
+enum class LoadOrStore : bool { kLoad, kStore }; | ||
+ | ||
+// The purpose of this class is to reconstruct the PC offset of a protected | ||
+// instruction (load or store) that has just been emitted. We cannot simply | ||
+// record the current PC offset before emitting the instruction, because the | ||
+// respective helper function we call might emit more than one instruction | ||
+// (e.g. to load an immediate into a register, or to get a constant pool | ||
+// out of the way). | ||
+// | ||
+// Template arguments: | ||
+// kLoadOrStore: | ||
+// DCHECK that the detected protected instruction has the right type. | ||
+// kExtraEmittedInstructions: | ||
+// By default, we assume that when the destructor runs, the PC is right | ||
+// behind the protected instruction. If additional instructions are expected | ||
+// to have been emitted (such as a pointer decompression), specify their | ||
+// number here. | ||
+template <LoadOrStore kLoadOrStore, uint8_t kExtraEmittedInstructions = 0> | ||
+class GetProtectedInstruction { | ||
+ public: | ||
+ GetProtectedInstruction(LiftoffAssembler* assm, | ||
+ uint32_t* protected_instruction_pc) | ||
+ : assm_(assm), | ||
+ protected_instruction_pc_(protected_instruction_pc), | ||
+ // First emit any required pools... | ||
+ blocked_pools_scope_(assm, kReservedInstructions * kInstrSize), | ||
+ // ...then record the PC offset before the relevant instruction | ||
+ // sequence. | ||
+ previous_pc_offset_(assm->pc_offset()) {} | ||
+ | ||
+ ~GetProtectedInstruction() { | ||
+ if (!protected_instruction_pc_) return; | ||
+ *protected_instruction_pc_ = | ||
+ assm_->pc_offset() - kInstrSize * (1 + kExtraEmittedInstructions); | ||
+ if constexpr (kLoadOrStore == LoadOrStore::kLoad) { | ||
+ DCHECK(assm_->InstructionAt(*protected_instruction_pc_)->IsLoad()); | ||
+ } else { | ||
+ DCHECK(assm_->InstructionAt(*protected_instruction_pc_)->IsStore()); | ||
+ } | ||
+ // Make sure {kReservedInstructions} was large enough. | ||
+ DCHECK_LE(assm_->pc_offset() - previous_pc_offset_, | ||
+ kReservedInstructions * kInstrSize); | ||
+ USE(previous_pc_offset_); | ||
+ } | ||
+ | ||
+ private: | ||
+ // For simplicity, we hard-code this value. We could make it a template | ||
+ // argument if we needed more flexibility. It must be at least the maximum | ||
+ // length of the instruction sequence emitted by the {LoadTaggedField} etc. | ||
+ // helper functions below. | ||
+ static constexpr int kReservedInstructions = 4; | ||
+ | ||
+ LiftoffAssembler* assm_; | ||
+ uint32_t* protected_instruction_pc_; | ||
+ MacroAssembler::BlockPoolsScope blocked_pools_scope_; | ||
+ int previous_pc_offset_; | ||
+}; | ||
+ | ||
void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr, | ||
Register offset_reg, | ||
int32_t offset_imm, | ||
@@ -553,19 +611,11 @@ void LiftoffAssembler::LoadTaggedPointer(Register dst, Register src_addr, | ||
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); | ||
+ DCHECK(!src_op.IsPostIndex()); // See MacroAssembler::LoadStoreMacroComplex. | ||
+ constexpr uint8_t kDecompressionInstruction = COMPRESS_POINTERS_BOOL ? 1 : 0; | ||
+ GetProtectedInstruction<LoadOrStore::kLoad, kDecompressionInstruction> | ||
+ collect_protected_load(this, protected_load_pc); | ||
LoadTaggedField(dst, src_op); | ||
- | ||
- // Since LoadTaggedField might start with an instruction loading an immediate | ||
- // argument to a register, we have to compute the {protected_load_pc} after | ||
- // calling it. | ||
- // In case of compressed pointers, there is an additional instruction | ||
- // (pointer decompression) after the load. | ||
- uint8_t protected_instruction_offset_bias = | ||
- COMPRESS_POINTERS_BOOL ? 2 * kInstrSize : kInstrSize; | ||
- if (protected_load_pc) { | ||
- *protected_load_pc = pc_offset() - protected_instruction_offset_bias; | ||
- DCHECK(InstructionAt(*protected_load_pc)->IsLoad()); | ||
- } | ||
} | ||
|
||
void LiftoffAssembler::LoadProtectedPointer(Register dst, Register src_addr, | ||
@@ -602,23 +652,19 @@ void LiftoffAssembler::StoreTaggedPointer(Register dst_addr, | ||
UseScratchRegisterScope temps(this); | ||
Operand offset_op = offset_reg.is_valid() ? Operand(offset_reg.W(), UXTW) | ||
: Operand(offset_imm); | ||
- // For the write barrier (below), we cannot have both an offset register and | ||
- // an immediate offset. Add them to a 32-bit offset initially, but in a 64-bit | ||
- // register, because that's needed in the MemOperand below. | ||
+ // This is similar to {liftoff::GetMemOp}, but leaves {dst_addr} alone, and | ||
+ // gives us a combined {offset_op}, which we need for the write barrier | ||
+ // below. The 32-bit addition is okay because on-heap offsets don't get | ||
+ // bigger than that. | ||
if (offset_reg.is_valid() && offset_imm) { | ||
Register effective_offset = temps.AcquireX(); | ||
Add(effective_offset.W(), offset_reg.W(), offset_imm); | ||
offset_op = effective_offset; | ||
} | ||
- | ||
- StoreTaggedField(src, MemOperand(dst_addr.X(), offset_op)); | ||
- | ||
- // Since StoreTaggedField might start with an instruction loading an immediate | ||
- // argument to a register, we have to compute the {protected_load_pc} after | ||
- // calling it. | ||
- if (protected_store_pc) { | ||
- *protected_store_pc = pc_offset() - kInstrSize; | ||
- DCHECK(InstructionAt(*protected_store_pc)->IsStore()); | ||
+ { | ||
+ GetProtectedInstruction<LoadOrStore::kStore> collect_protected_store( | ||
+ this, protected_store_pc); | ||
+ StoreTaggedField(src, MemOperand(dst_addr.X(), offset_op)); | ||
} | ||
|
||
if (skip_write_barrier || v8_flags.disable_write_barriers) return; | ||
@@ -643,6 +689,9 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr, | ||
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, shift_amount); | ||
+ DCHECK(!src_op.IsPostIndex()); // See MacroAssembler::LoadStoreMacroComplex. | ||
+ GetProtectedInstruction<LoadOrStore::kLoad> collect_protected_load( | ||
+ this, protected_load_pc); | ||
switch (type.value()) { | ||
case LoadType::kI32Load8U: | ||
case LoadType::kI64Load8U: | ||
@@ -684,13 +733,6 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr, | ||
Ldr(dst.fp().Q(), src_op); | ||
break; | ||
} | ||
- // Since {Ldr*} macros might start with an instruction loading an immediate | ||
- // argument to a register, we have to compute the {protected_load_pc} after | ||
- // calling them. | ||
- if (protected_load_pc) { | ||
- *protected_load_pc = pc_offset() - kInstrSize; | ||
- DCHECK(InstructionAt(*protected_load_pc)->IsLoad()); | ||
- } | ||
} | ||
|
||
void LiftoffAssembler::Store(Register dst_addr, Register offset_reg, | ||
@@ -701,6 +743,9 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg, | ||
UseScratchRegisterScope temps(this); | ||
MemOperand dst_op = liftoff::GetMemOp(this, &temps, dst_addr, offset_reg, | ||
offset_imm, i64_offset); | ||
+ DCHECK(!dst_op.IsPostIndex()); // See MacroAssembler::LoadStoreMacroComplex. | ||
+ GetProtectedInstruction<LoadOrStore::kStore> collect_protected_store( | ||
+ this, protected_store_pc); | ||
switch (type.value()) { | ||
case StoreType::kI32Store8: | ||
case StoreType::kI64Store8: | ||
@@ -727,13 +772,6 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg, | ||
Str(src.fp().Q(), dst_op); | ||
break; | ||
} | ||
- // Since {Str*} macros might start with an instruction loading an immediate | ||
- // argument to a register, we have to compute the {protected_load_pc} after | ||
- // calling them. | ||
- if (protected_store_pc) { | ||
- *protected_store_pc = pc_offset() - kInstrSize; | ||
- DCHECK(InstructionAt(*protected_store_pc)->IsStore()); | ||
- } | ||
} | ||
|
||
namespace liftoff { | ||
diff --git a/test/mjsunit/regress/wasm/regress-325359458.js b/test/mjsunit/regress/wasm/regress-325359458.js | ||
new file mode 100644 | ||
index 0000000000000000000000000000000000000000..57b71ecd0f9139f41ea103ef56a1cac6a571e968 | ||
--- /dev/null | ||
+++ b/test/mjsunit/regress/wasm/regress-325359458.js | ||
@@ -0,0 +1,23 @@ | ||
+// Copyright 2024 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. | ||
+ | ||
+d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js'); | ||
+ | ||
+let builder = new WasmModuleBuilder(); | ||
+let struct = builder.addStruct([makeField(kWasmAnyRef, true)]); | ||
+ | ||
+let body = [kExprRefNull, struct]; | ||
+for (let i = 0; i < 800; i++) { | ||
+ body = body.concat(...[ | ||
+ kGCPrefix, kExprStructGet, struct, 0, | ||
+ kGCPrefix, kExprRefCastNull, struct, | ||
+ ]); | ||
+} | ||
+body = body.concat(...[kExprDrop]); | ||
+builder.addFunction("main", kSig_v_v).exportFunc().addBody(body); | ||
+ | ||
+let instance = builder.instantiate(); | ||
+// Just check if we can compile without DCHECK failures. | ||
+assertThrows(() => instance.exports.main(), WebAssembly.RuntimeError, | ||
+ 'dereferencing a null pointer'); |