Skip to content

Commit aff53dd

Browse files
committed
deps: V8: cherry-pick 482e5c7750b3
Original commit message: Merged: [wasm-simd] Fix loading fp pair registers We were incorrectly clearing the high reg from the list of regs to load. The intention was to prevent double (and incorrect) loading - loading 128 bits from the low fp and the loading 128 bits from the high fp. But this violates the assumption that the two regs in a pair would be set or unset at the same time. The fix here is to introduce a new enum for register loads, a nop, which does nothing. The high fp of the fp pair will be tied to this nop, so as we iterate down the reglist, we load 128 bits using the low fp, then don't load anything for the high fp. Bug: chromium:1161654 (cherry picked from commit 8c698702ced0de085aa91370d8cb44deab3fcf54) (cherry picked from commit ffd6ff5a61b9343ccc62e6c03b71a33682c6084d) Change-Id: Ib8134574b24f74f24ca9efd34b3444173296d8f1 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2619416 Commit-Queue: Zhi An Ng <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Cr-Original-Commit-Position: refs/branch-heads/8.8@{#28} Cr-Original-Branched-From: 2dbcdc105b963ee2501c82139eef7e0603977ff0-refs/heads/8.8.278@{#1} Cr-Original-Branched-From: 366d30c99049b3f1c673f8a93deb9f879d0fa9f0-refs/heads/master@{#71094} Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649176 Reviewed-by: Victor-Gabriel Savu <[email protected]> Commit-Queue: Achuith Bhandarkar <[email protected]> Cr-Commit-Position: refs/branch-heads/8.6@{#55} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} Refs: v8/v8@482e5c7 PR-URL: #38275 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>
1 parent 931d31a commit aff53dd

File tree

3 files changed

+68
-5
lines changed

3 files changed

+68
-5
lines changed

common.gypi

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737
# Reset this number to 0 on major V8 upgrades.
3838
# Increment by one for each non-official patch applied to deps/v8.
39-
'v8_embedder_string': '-node.40',
39+
'v8_embedder_string': '-node.41',
4040

4141
##### V8 defaults for Node.js #####
4242

deps/v8/src/wasm/baseline/liftoff-assembler.cc

+11-4
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ class StackTransferRecipe {
3737

3838
struct RegisterLoad {
3939
enum LoadKind : uint8_t {
40+
kNop, // no-op, used for high fp of a fp pair.
4041
kConstant, // load a constant value into a register.
4142
kStack, // fill a register from a stack slot.
4243
kLowHalfStack, // fill a register from the low half of a stack slot.
@@ -63,6 +64,10 @@ class StackTransferRecipe {
6364
return {half == kLowWord ? kLowHalfStack : kHighHalfStack, kWasmI32,
6465
offset};
6566
}
67+
static RegisterLoad Nop() {
68+
// ValueType does not matter.
69+
return {kNop, kWasmI32, 0};
70+
}
6671

6772
private:
6873
RegisterLoad(LoadKind kind, ValueType type, int32_t value)
@@ -217,11 +222,11 @@ class StackTransferRecipe {
217222
RegisterLoad::HalfStack(stack_offset, kHighWord);
218223
} else if (dst.is_fp_pair()) {
219224
DCHECK_EQ(kWasmS128, type);
220-
// load_dst_regs_.set above will set both low and high fp regs.
221-
// But unlike gp_pair, we load a kWasm128 in one go in ExecuteLoads.
222-
// So unset the top fp register to skip loading it.
223-
load_dst_regs_.clear(dst.high());
225+
// Only need register_load for low_gp since we load 128 bits at one go.
226+
// Both low and high need to be set in load_dst_regs_ but when iterating
227+
// over it, both low and high will be cleared, so we won't load twice.
224228
*register_load(dst.low()) = RegisterLoad::Stack(stack_offset, type);
229+
*register_load(dst.high()) = RegisterLoad::Nop();
225230
} else {
226231
*register_load(dst) = RegisterLoad::Stack(stack_offset, type);
227232
}
@@ -318,6 +323,8 @@ class StackTransferRecipe {
318323
for (LiftoffRegister dst : load_dst_regs_) {
319324
RegisterLoad* load = register_load(dst);
320325
switch (load->kind) {
326+
case RegisterLoad::kNop:
327+
break;
321328
case RegisterLoad::kConstant:
322329
asm_->LoadConstant(dst, load->type == kWasmI64
323330
? WasmValue(int64_t{load->value})
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --wasm-staging
6+
7+
// This is a fuzzer-generated test case that exposed a bug in Liftoff that only
8+
// affects ARM, where the fp register aliasing is different from other archs.
9+
// We were inncorrectly clearing the the high fp register in a LiftoffRegList
10+
// indicating registers to load, hitting a DCHECK.
11+
load('test/mjsunit/wasm/wasm-module-builder.js');
12+
13+
const builder = new WasmModuleBuilder();
14+
builder.addMemory(19, 32, false);
15+
builder.addGlobal(kWasmI32, 0);
16+
builder.addType(makeSig([], []));
17+
builder.addType(makeSig([kWasmI64, kWasmS128, kWasmF32], [kWasmI32]));
18+
// Generate function 1 (out of 5).
19+
builder.addFunction(undefined, 0 /* sig */)
20+
.addBodyWithEnd([
21+
// signature: v_v
22+
// body:
23+
kExprI32Const, 0x05, // i32.const
24+
kExprReturn, // return
25+
kExprUnreachable, // unreachable
26+
kExprEnd, // end @5
27+
]);
28+
// Generate function 4 (out of 5).
29+
builder.addFunction(undefined, 1 /* sig */)
30+
.addBodyWithEnd([
31+
// signature: i_lsf
32+
// body:
33+
kExprLocalGet, 0x01, // local.get
34+
kExprLocalGet, 0x01, // local.get
35+
kExprGlobalGet, 0x00, // global.get
36+
kExprDrop, // drop
37+
kExprLoop, kWasmStmt, // loop @8
38+
kExprLoop, 0x00, // loop @10
39+
kExprI32Const, 0x01, // i32.const
40+
kExprMemoryGrow, 0x00, // memory.grow
41+
kExprI64LoadMem8U, 0x00, 0x70, // i64.load8_u
42+
kExprLoop, 0x00, // loop @19
43+
kExprCallFunction, 0x00, // call function #0: v_v
44+
kExprEnd, // end @23
45+
kExprI64Const, 0xf1, 0x24, // i64.const
46+
kExprGlobalGet, 0x00, // global.get
47+
kExprDrop, // drop
48+
kExprBr, 0x00, // br depth=0
49+
kExprEnd, // end @32
50+
kExprEnd, // end @33
51+
kExprI32Const, 0x5b, // i32.const
52+
kExprReturn, // return
53+
kExprEnd, // end @37
54+
]);
55+
// Instantiation is enough to cause a crash.
56+
const instance = builder.instantiate();

0 commit comments

Comments
 (0)