Skip to content

Commit

Permalink
deps: V8: cherry-pick 482e5c7750b3
Browse files Browse the repository at this point in the history
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 <zhin@chromium.org>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Original-Commit-Position: refs/branch-heads/8.8@{nodejs#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 <vsavu@google.com>
    Commit-Queue: Achuith Bhandarkar <achuith@chromium.org>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#55}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@482e5c7
  • Loading branch information
targos committed Apr 17, 2021
1 parent 38d1e4c commit 6e0a139
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 5 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

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

##### V8 defaults for Node.js #####

Expand Down
15 changes: 11 additions & 4 deletions deps/v8/src/wasm/baseline/liftoff-assembler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class StackTransferRecipe {

struct RegisterLoad {
enum LoadKind : uint8_t {
kNop, // no-op, used for high fp of a fp pair.
kConstant, // load a constant value into a register.
kStack, // fill a register from a stack slot.
kLowHalfStack, // fill a register from the low half of a stack slot.
Expand All @@ -63,6 +64,10 @@ class StackTransferRecipe {
return {half == kLowWord ? kLowHalfStack : kHighHalfStack, kWasmI32,
offset};
}
static RegisterLoad Nop() {
// ValueType does not matter.
return {kNop, kWasmI32, 0};
}

private:
RegisterLoad(LoadKind kind, ValueType type, int32_t value)
Expand Down Expand Up @@ -217,11 +222,11 @@ class StackTransferRecipe {
RegisterLoad::HalfStack(stack_offset, kHighWord);
} else if (dst.is_fp_pair()) {
DCHECK_EQ(kWasmS128, type);
// load_dst_regs_.set above will set both low and high fp regs.
// But unlike gp_pair, we load a kWasm128 in one go in ExecuteLoads.
// So unset the top fp register to skip loading it.
load_dst_regs_.clear(dst.high());
// Only need register_load for low_gp since we load 128 bits at one go.
// Both low and high need to be set in load_dst_regs_ but when iterating
// over it, both low and high will be cleared, so we won't load twice.
*register_load(dst.low()) = RegisterLoad::Stack(stack_offset, type);
*register_load(dst.high()) = RegisterLoad::Nop();
} else {
*register_load(dst) = RegisterLoad::Stack(stack_offset, type);
}
Expand Down Expand Up @@ -318,6 +323,8 @@ class StackTransferRecipe {
for (LiftoffRegister dst : load_dst_regs_) {
RegisterLoad* load = register_load(dst);
switch (load->kind) {
case RegisterLoad::kNop:
break;
case RegisterLoad::kConstant:
asm_->LoadConstant(dst, load->type == kWasmI64
? WasmValue(int64_t{load->value})
Expand Down
56 changes: 56 additions & 0 deletions deps/v8/test/mjsunit/regress/wasm/regress-1161654.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// 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.

// Flags: --wasm-staging

// This is a fuzzer-generated test case that exposed a bug in Liftoff that only
// affects ARM, where the fp register aliasing is different from other archs.
// We were inncorrectly clearing the the high fp register in a LiftoffRegList
// indicating registers to load, hitting a DCHECK.
load('test/mjsunit/wasm/wasm-module-builder.js');

const builder = new WasmModuleBuilder();
builder.addMemory(19, 32, false);
builder.addGlobal(kWasmI32, 0);
builder.addType(makeSig([], []));
builder.addType(makeSig([kWasmI64, kWasmS128, kWasmF32], [kWasmI32]));
// Generate function 1 (out of 5).
builder.addFunction(undefined, 0 /* sig */)
.addBodyWithEnd([
// signature: v_v
// body:
kExprI32Const, 0x05, // i32.const
kExprReturn, // return
kExprUnreachable, // unreachable
kExprEnd, // end @5
]);
// Generate function 4 (out of 5).
builder.addFunction(undefined, 1 /* sig */)
.addBodyWithEnd([
// signature: i_lsf
// body:
kExprLocalGet, 0x01, // local.get
kExprLocalGet, 0x01, // local.get
kExprGlobalGet, 0x00, // global.get
kExprDrop, // drop
kExprLoop, kWasmStmt, // loop @8
kExprLoop, 0x00, // loop @10
kExprI32Const, 0x01, // i32.const
kExprMemoryGrow, 0x00, // memory.grow
kExprI64LoadMem8U, 0x00, 0x70, // i64.load8_u
kExprLoop, 0x00, // loop @19
kExprCallFunction, 0x00, // call function #0: v_v
kExprEnd, // end @23
kExprI64Const, 0xf1, 0x24, // i64.const
kExprGlobalGet, 0x00, // global.get
kExprDrop, // drop
kExprBr, 0x00, // br depth=0
kExprEnd, // end @32
kExprEnd, // end @33
kExprI32Const, 0x5b, // i32.const
kExprReturn, // return
kExprEnd, // end @37
]);
// Instantiation is enough to cause a crash.
const instance = builder.instantiate();

0 comments on commit 6e0a139

Please sign in to comment.