Skip to content

Commit 22a16bd

Browse files
backesV8 LUCI CQ
backes
authored and
V8 LUCI CQ
committed
[wasm] Fix return value of lazy compile runtime function
The Runtime_WasmCompileLazy function was returning a ptr-sized address, wrapped in an Object. This worked because no GC is triggered between the return from the runtime function and the point where we jump to the returned address. In a pointer-compressed world though, generated code assumes that all objects live in the same 4GB heap, so comparisons only compare the lower 32 bit. On a 64-bit system, this can lead to collisions where a comparison determines that the returned address equals a heap object, even though the upper 32-bit differ. This happens occasionally in the wild, where the returned function entry pointer has the same lower half than the exception sentinel value. This leads to triggering stack unwinding (by the CEntry stub), which then fails (with a CHECK) because there is no pending exception. This CL fixes that by returning a Smi instead which is the offset in the jump table where the kWasmCompileLazy builtin should jump to. The builtin then gets the jump table start address from the instance object, adds the offset that the runtime function returned, and performs the jump. We do not include a regression test because this failure is very spurious and hard to reproduce. [email protected] Bug: chromium:1311960 Change-Id: I5a72daf78905904f8ae8ade8630793c42e223984 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3663093 Commit-Queue: Clemens Backes <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Cr-Commit-Position: refs/heads/main@{#80729}
1 parent 9964283 commit 22a16bd

6 files changed

Lines changed: 106 additions & 58 deletions

File tree

‎src/builtins/arm/builtins-arm.cc‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2666,8 +2666,7 @@ void Builtins::Generate_Construct(MacroAssembler* masm) {
26662666
void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
26672667
// The function index was put in a register by the jump table trampoline.
26682668
// Convert to Smi for the runtime call.
2669-
__ SmiTag(kWasmCompileLazyFuncIndexRegister,
2670-
kWasmCompileLazyFuncIndexRegister);
2669+
__ SmiTag(kWasmCompileLazyFuncIndexRegister);
26712670
{
26722671
HardAbortScope hard_abort(masm); // Avoid calls to Abort.
26732672
FrameAndConstantPoolScope scope(masm, StackFrame::WASM_COMPILE_LAZY);
@@ -2697,22 +2696,34 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
26972696
__ stm(db_w, sp, gp_regs);
26982697
__ vstm(db_w, sp, lowest_fp_reg, highest_fp_reg);
26992698

2700-
// Pass instance and function index as explicit arguments to the runtime
2699+
// Push the Wasm instance for loading the jump table address after the
2700+
// runtime call.
2701+
__ push(kWasmInstanceRegister);
2702+
2703+
// Push the Wasm instance again as an explicit argument to the runtime
27012704
// function.
27022705
__ push(kWasmInstanceRegister);
2706+
// Push the function index as second argument.
27032707
__ push(kWasmCompileLazyFuncIndexRegister);
27042708
// Initialize the JavaScript context with 0. CEntry will use it to
27052709
// set the current context on the isolate.
27062710
__ Move(cp, Smi::zero());
27072711
__ CallRuntime(Runtime::kWasmCompileLazy, 2);
2708-
// The entrypoint address is the return value.
2709-
__ mov(r8, kReturnRegister0);
2712+
// The runtime function returns the jump table slot offset as a Smi. Use
2713+
// that to compute the jump target in r8.
2714+
__ pop(kWasmInstanceRegister);
2715+
__ ldr(r8, MemOperand(
2716+
kWasmInstanceRegister,
2717+
WasmInstanceObject::kJumpTableStartOffset - kHeapObjectTag));
2718+
__ add(r8, r8, Operand::SmiUntag(kReturnRegister0));
2719+
// r8 now holds the jump table slot where we want to jump to in the end.
27102720

27112721
// Restore registers.
27122722
__ vldm(ia_w, sp, lowest_fp_reg, highest_fp_reg);
27132723
__ ldm(ia_w, sp, gp_regs);
27142724
}
2715-
// Finally, jump to the entrypoint.
2725+
2726+
// Finally, jump to the jump table slot for the function.
27162727
__ Jump(r8);
27172728
}
27182729

‎src/builtins/arm64/builtins-arm64.cc‎

Lines changed: 48 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3056,41 +3056,50 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
30563056
// Sign extend and convert to Smi for the runtime call.
30573057
__ sxtw(kWasmCompileLazyFuncIndexRegister,
30583058
kWasmCompileLazyFuncIndexRegister.W());
3059-
__ SmiTag(kWasmCompileLazyFuncIndexRegister,
3060-
kWasmCompileLazyFuncIndexRegister);
3061-
3062-
UseScratchRegisterScope temps(masm);
3063-
{
3064-
HardAbortScope hard_abort(masm); // Avoid calls to Abort.
3065-
FrameScope scope(masm, StackFrame::WASM_COMPILE_LAZY);
3066-
3067-
// Save all parameter registers (see wasm-linkage.h). They might be
3068-
// overwritten in the runtime call below. We don't have any callee-saved
3069-
// registers in wasm, so no need to store anything else.
3070-
RegList gp_regs;
3059+
__ SmiTag(kWasmCompileLazyFuncIndexRegister);
3060+
3061+
// Compute register lists for parameters to be saved. We save all parameter
3062+
// registers (see wasm-linkage.h). They might be overwritten in the runtime
3063+
// call below. We don't have any callee-saved registers in wasm, so no need to
3064+
// store anything else.
3065+
constexpr RegList kSavedGpRegs = ([]() constexpr {
3066+
RegList saved_gp_regs;
30713067
for (Register gp_param_reg : wasm::kGpParamRegisters) {
3072-
gp_regs.set(gp_param_reg);
3068+
saved_gp_regs.set(gp_param_reg);
30733069
}
30743070
// Also push x1, because we must push multiples of 16 bytes (see
30753071
// {TurboAssembler::PushCPURegList}.
3076-
CHECK_EQ(1, gp_regs.Count() % 2);
3077-
gp_regs.set(x1);
3078-
CHECK_EQ(0, gp_regs.Count() % 2);
3072+
saved_gp_regs.set(x1);
3073+
// All set registers were unique.
3074+
CHECK_EQ(saved_gp_regs.Count(), arraysize(wasm::kGpParamRegisters) + 1);
3075+
// We push a multiple of 16 bytes.
3076+
CHECK_EQ(0, saved_gp_regs.Count() % 2);
3077+
// The Wasm instance must be part of the saved registers.
3078+
CHECK(saved_gp_regs.has(kWasmInstanceRegister));
3079+
CHECK_EQ(WasmCompileLazyFrameConstants::kNumberOfSavedGpParamRegs,
3080+
saved_gp_regs.Count());
3081+
return saved_gp_regs;
3082+
})();
30793083

3080-
DoubleRegList fp_regs;
3084+
constexpr DoubleRegList kSavedFpRegs = ([]() constexpr {
3085+
DoubleRegList saved_fp_regs;
30813086
for (DoubleRegister fp_param_reg : wasm::kFpParamRegisters) {
3082-
fp_regs.set(fp_param_reg);
3087+
saved_fp_regs.set(fp_param_reg);
30833088
}
30843089

3085-
CHECK_EQ(gp_regs.Count(), arraysize(wasm::kGpParamRegisters) + 1);
3086-
CHECK_EQ(fp_regs.Count(), arraysize(wasm::kFpParamRegisters));
3087-
CHECK_EQ(WasmCompileLazyFrameConstants::kNumberOfSavedGpParamRegs,
3088-
gp_regs.Count());
3090+
CHECK_EQ(saved_fp_regs.Count(), arraysize(wasm::kFpParamRegisters));
30893091
CHECK_EQ(WasmCompileLazyFrameConstants::kNumberOfSavedFpParamRegs,
3090-
fp_regs.Count());
3092+
saved_fp_regs.Count());
3093+
return saved_fp_regs;
3094+
})();
30913095

3092-
__ PushXRegList(gp_regs);
3093-
__ PushQRegList(fp_regs);
3096+
{
3097+
HardAbortScope hard_abort(masm); // Avoid calls to Abort.
3098+
FrameScope scope(masm, StackFrame::WASM_COMPILE_LAZY);
3099+
3100+
// Save registers that we need to keep alive across the runtime call.
3101+
__ PushXRegList(kSavedGpRegs);
3102+
__ PushQRegList(kSavedFpRegs);
30943103

30953104
// Pass instance and function index as explicit arguments to the runtime
30963105
// function.
@@ -3100,17 +3109,23 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
31003109
__ Mov(cp, Smi::zero());
31013110
__ CallRuntime(Runtime::kWasmCompileLazy, 2);
31023111

3103-
// Exclude x17 from the scope, there are hardcoded uses of it below.
3104-
temps.Exclude(x17);
3105-
3106-
// The entrypoint address is the return value.
3107-
__ Mov(x17, kReturnRegister0);
3112+
// Untag the returned Smi into into x17, for later use.
3113+
static_assert(!kSavedGpRegs.has(x17));
3114+
__ SmiUntag(x17, kReturnRegister0);
31083115

31093116
// Restore registers.
3110-
__ PopQRegList(fp_regs);
3111-
__ PopXRegList(gp_regs);
3117+
__ PopQRegList(kSavedFpRegs);
3118+
__ PopXRegList(kSavedGpRegs);
31123119
}
3113-
// Finally, jump to the entrypoint.
3120+
3121+
// The runtime function returned the jump table slot offset as a Smi (now in
3122+
// x17). Use that to compute the jump target.
3123+
static_assert(!kSavedGpRegs.has(x18));
3124+
__ ldr(x18, MemOperand(
3125+
kWasmInstanceRegister,
3126+
WasmInstanceObject::kJumpTableStartOffset - kHeapObjectTag));
3127+
__ add(x17, x18, Operand(x17));
3128+
// Finally, jump to the jump table slot for the function.
31143129
__ Jump(x17);
31153130
}
31163131

‎src/builtins/ia32/builtins-ia32.cc‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2933,20 +2933,28 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
29332933
offset += kSimd128Size;
29342934
}
29352935

2936-
// Push the Wasm instance as an explicit argument to WasmCompileLazy.
2936+
// Push the Wasm instance for loading the jump table address after the
2937+
// runtime call.
2938+
__ Push(kWasmInstanceRegister);
2939+
2940+
// Push the Wasm instance again as an explicit argument to the runtime
2941+
// function.
29372942
__ Push(kWasmInstanceRegister);
29382943
// Push the function index as second argument.
29392944
__ Push(kWasmCompileLazyFuncIndexRegister);
29402945
// Initialize the JavaScript context with 0. CEntry will use it to
29412946
// set the current context on the isolate.
29422947
__ Move(kContextRegister, Smi::zero());
2943-
{
2944-
// At this point, ebx has been spilled to the stack but is not yet
2945-
// overwritten with another value. We can still use it as kRootRegister.
2946-
__ CallRuntime(Runtime::kWasmCompileLazy, 2);
2947-
}
2948-
// The entrypoint address is the return value.
2949-
__ mov(edi, kReturnRegister0);
2948+
__ CallRuntime(Runtime::kWasmCompileLazy, 2);
2949+
// The runtime function returns the jump table slot offset as a Smi. Use
2950+
// that to compute the jump target in edi.
2951+
__ Pop(kWasmInstanceRegister);
2952+
__ mov(edi, MemOperand(kWasmInstanceRegister,
2953+
WasmInstanceObject::kJumpTableStartOffset -
2954+
kHeapObjectTag));
2955+
__ SmiUntag(kReturnRegister0);
2956+
__ add(edi, kReturnRegister0);
2957+
// edi now holds the jump table slot where we want to jump to in the end.
29502958

29512959
// Restore registers.
29522960
for (DoubleRegister reg : base::Reversed(wasm::kFpParamRegisters)) {
@@ -2959,7 +2967,8 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
29592967
__ Pop(reg);
29602968
}
29612969
}
2962-
// Finally, jump to the entrypoint.
2970+
2971+
// Finally, jump to the jump table slot for the function.
29632972
__ jmp(edi);
29642973
}
29652974

‎src/builtins/x64/builtins-x64.cc‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2816,6 +2816,7 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
28162816
__ Pop(r15);
28172817
// Convert to Smi for the runtime call.
28182818
__ SmiTag(r15);
2819+
28192820
{
28202821
HardAbortScope hard_abort(masm); // Avoid calls to Abort.
28212822
FrameScope scope(masm, StackFrame::WASM_COMPILE_LAZY);
@@ -2839,16 +2840,28 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
28392840
offset += kSimd128Size;
28402841
}
28412842

2842-
// Push the Wasm instance as an explicit argument to WasmCompileLazy.
2843+
// Push the Wasm instance for loading the jump table address after the
2844+
// runtime call.
2845+
__ Push(kWasmInstanceRegister);
2846+
2847+
// Push the Wasm instance again as an explicit argument to the runtime
2848+
// function.
28432849
__ Push(kWasmInstanceRegister);
28442850
// Push the function index as second argument.
28452851
__ Push(r15);
28462852
// Initialize the JavaScript context with 0. CEntry will use it to
28472853
// set the current context on the isolate.
28482854
__ Move(kContextRegister, Smi::zero());
28492855
__ CallRuntime(Runtime::kWasmCompileLazy, 2);
2850-
// The entrypoint address is the return value.
2851-
__ movq(r15, kReturnRegister0);
2856+
// The runtime function returns the jump table slot offset as a Smi. Use
2857+
// that to compute the jump target in r15.
2858+
__ Pop(kWasmInstanceRegister);
2859+
__ movq(r15, MemOperand(kWasmInstanceRegister,
2860+
wasm::ObjectAccess::ToTagged(
2861+
WasmInstanceObject::kJumpTableStartOffset)));
2862+
__ SmiUntag(kReturnRegister0);
2863+
__ addq(r15, kReturnRegister0);
2864+
// r15 now holds the jump table slot where we want to jump to in the end.
28522865

28532866
// Restore registers.
28542867
for (DoubleRegister reg : base::Reversed(wasm::kFpParamRegisters)) {
@@ -2861,7 +2874,8 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
28612874
__ Pop(reg);
28622875
}
28632876
}
2864-
// Finally, jump to the entrypoint.
2877+
2878+
// Finally, jump to the jump table slot for the function.
28652879
__ jmp(r15);
28662880
}
28672881

‎src/runtime/runtime-wasm.cc‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -237,14 +237,11 @@ RUNTIME_FUNCTION(Runtime_WasmCompileLazy) {
237237
isolate, instance->module_object().native_module(), func_index);
238238
}
239239
DCHECK(isolate->has_pending_exception());
240-
return ReadOnlyRoots(isolate).exception();
240+
return ReadOnlyRoots{isolate}.exception();
241241
}
242242

243-
Address entrypoint =
244-
instance->module_object().native_module()->GetCallTargetForFunction(
245-
func_index);
246-
247-
return Object(entrypoint);
243+
auto* native_module = instance->module_object().native_module();
244+
return Smi::FromInt(native_module->GetJumpTableOffset(func_index));
248245
}
249246

250247
namespace {

‎src/wasm/wasm-linkage.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,8 @@ constexpr DoubleRegister kFpReturnRegisters[] = {};
143143
// The parameter index where the instance parameter should be placed in wasm
144144
// call descriptors. This is used by the Int64Lowering::LowerNode method.
145145
constexpr int kWasmInstanceParameterIndex = 0;
146+
static_assert(kWasmInstanceRegister ==
147+
kGpParamRegisters[kWasmInstanceParameterIndex]);
146148

147149
class LinkageAllocator {
148150
public:

0 commit comments

Comments
 (0)