Skip to content

V
V8
Commit 1f89ee1f authored by Jakob Gruber's avatar Jakob Gruber Committed by V8 LUCI CQ
Browse files
Options

Revert "Reland "[wasm][bug] Fix a couple of bugs in validation of unreachable code"" 

This reverts commit 916eb869.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux%20gcc/11805/overview

Original change's description:
> Reland "[wasm][bug] Fix a couple of bugs in validation of unreachable code"
>
> This is a reland of 4a037f87
>
> Changes compared to original change: None. This seems not to create
> problems after all.
>
> Original change's description:
> > [wasm][bug] Fix a couple of bugs in validation of unreachable code
> >
> > Changes:
> > - SetBlockType now instantiates the block's start merge with values of
> >   the correct type in unreachable code.
> > - EnsureStackArguments now keeps the existing stack values and moves
> >   them over the new bottom values.
> > - Drop stack size validation in Drop().
> > - Add new tests in unreachable-validation.js.
> >
> > Change-Id: Ie68b3d9abb0a41d1623d4a123fb526e71941c4e7
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2902733
> > Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
> > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#74650}
>
> Change-Id: Id620f7fb6677b772b0dcfd38108256384db44439
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2905598
> Reviewed-by: Manos Koukoutos <manoskouk@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74677}

Tbr: manoskouk@chromium.org
Change-Id: Ia24aa453735464bdd3aafca4617beabb0cbf8823
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2917601
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: 's avatarClemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74771}
parent a58f812c
Hide whitespace changes
Inline Side-by-side
Showing with 19 additions and 47 deletions
src/wasm/function-body-decoder-impl.h
View file @ 1f89ee1f
......@@ -3583,18 +3583,13 @@ class WasmFullDecoder : public WasmDecoder<validate> {
InitMerge(&c->end_merge, imm.out_arity(), [pc, &imm](uint32_t i) {
return Value{pc, imm.out_type(i)};
});
InitMerge(&c->start_merge, imm.in_arity(), [pc, &imm, args](uint32_t i) {
// The merge needs to be instantiated with Values of the correct type even
// in the presence of bottom values (i.e. in unreachable code). Since
// bottom Values will never be used for code generation, we can safely
// instantiate new ones in that case.
return args[i].type != kWasmBottom ? args[i] : Value{pc, imm.in_type(i)};
});
InitMerge(&c->start_merge, imm.in_arity(),
[args](uint32_t i) { return args[i]; });
}
V8_INLINE void EnsureStackArguments(int count) {
uint32_t limit = control_.back().stack_depth;
if (V8_LIKELY(stack_size() >= count + limit)) return;
if (stack_size() >= count + limit) return;
EnsureStackArguments_Slow(count, limit);
}
......@@ -3603,21 +3598,15 @@ class WasmFullDecoder : public WasmDecoder<validate> {
int index = count - stack_size() - 1;
NotEnoughArgumentsError(index);
}
// Silently create unreachable values out of thin air underneath the
// existing stack values. To do so, we have to move existing stack values
// upwards in the stack, then instantiate the new Values as
// {UnreachableValue}.
int current_values = stack_size() - limit;
int additional_values = count - current_values;
DCHECK_GT(additional_values, 0);
EnsureStackSpace(additional_values);
stack_end_ += additional_values;
Value* stack_base = stack_value(current_values + additional_values);
for (int i = current_values - 1; i >= 0; i--) {
stack_base[additional_values + i] = stack_base[i];
}
for (int i = 0; i < additional_values; i++) {
stack_base[i] = UnreachableValue(this->pc_);
// Silently create unreachable values out of thin air. Since we push them
// onto the stack, while conceptually we should be inserting them under
// any existing elements, we have to avoid validation failures that would
// be caused by finding non-unreachable values in the wrong slot, so we
// replace the entire current scope's values.
Drop(static_cast<int>(stack_size() - limit));
EnsureStackSpace(count + limit - stack_size());
while (stack_size() < count + limit) {
Push(UnreachableValue(this->pc_));
}
}
......@@ -3632,8 +3621,6 @@ class WasmFullDecoder : public WasmDecoder<validate> {
}
return args;
}
// Drops a number of stack elements equal to the {sig}'s parameter count (0 if
// {sig} is null), or all of them if less are present.
V8_INLINE void DropArgs(const FunctionSig* sig) {
int count = sig ? static_cast<int>(sig->parameter_count()) : 0;
Drop(count);
......@@ -3649,8 +3636,6 @@ class WasmFullDecoder : public WasmDecoder<validate> {
}
return args;
}
// Drops a number of stack elements equal to the struct's field count, or all
// of them if less are present.
V8_INLINE void DropArgs(const StructType* type) {
Drop(static_cast<int>(type->field_count()));
}
......@@ -4889,20 +4874,22 @@ class WasmFullDecoder : public WasmDecoder<validate> {
}
}
// Drop the top {count} stack elements, or all of them if less than {count}
// are present.
V8_INLINE void Drop(int count = 1) {
DCHECK(!control_.empty());
uint32_t limit = control_.back().stack_depth;
// TODO(wasm): This check is often redundant.
if (V8_UNLIKELY(stack_size() < limit + count)) {
// Popping past the current control start in reachable code.
if (!VALIDATE(!current_code_reachable_and_ok_)) {
NotEnoughArgumentsError(0);
}
// Pop what we can.
count = std::min(count, static_cast<int>(stack_size() - limit));
}
DCHECK_LE(stack_, stack_end_ - count);
stack_end_ -= count;
}
// Drop the top stack element if present. Takes a Value input for more
// descriptive call sites.
// For more descriptive call sites:
V8_INLINE void Drop(const Value& /* unused */) { Drop(1); }
enum StackElementsCountMode : bool {
......@@ -4935,9 +4922,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
: merge_type == kReturnMerge ? "return" : "fallthru";
uint32_t arity = merge->arity;
uint32_t actual = stack_size() - control_.back().stack_depth;
// Here we have to check for !unreachable(), because we need to typecheck as
// if the current code is reachable even if it is spec-only reachable.
if (V8_LIKELY(!control_.back().unreachable())) {
if (V8_LIKELY(current_code_reachable_and_ok_)) {
if (V8_UNLIKELY(strict_count ? actual != drop_values + arity
: actual < drop_values + arity)) {
this->DecodeError("expected %u elements on the stack for %s, found %u",
......
test/mjsunit/wasm/unreachable-validation.js
View file @ 1f89ee1f
......@@ -11,10 +11,6 @@ let unittest = true;
function run(expected, name, code) {
let builder = new WasmModuleBuilder();
// Index 0
builder.addType(kSig_i_i);
// Index 1
builder.addType(kSig_i_ii);
builder.addFunction("main", kSig_v_v).
addBody(code);
let buffer = builder.toBuffer();
......@@ -40,8 +36,6 @@ let X = undefined;
let nop = kExprNop;
let iadd = kExprI32Add;
let ieqz = kExprI32Eqz;
let leqz = kExprI64Eqz;
let unr = kExprUnreachable;
let ret = kExprReturn;
let br0 = [kExprBr, 0];
......@@ -51,7 +45,6 @@ let brt1 = [kExprBrTable, 0, 1];
let brt01 = [kExprBrTable, 1, 0, 1];
let f32 = [kExprF32Const, 0, 0, 0, 0];
let zero = [kExprI32Const, 0];
let zero64 = [kExprI64Const, 0];
let if_else_empty = [kExprIf, kWasmVoid, kExprElse, kExprEnd];
let if_unr = [kExprIf, kWasmVoid, kExprUnreachable, kExprEnd];
let if_else_unr = [kExprIf, kWasmVoid, kExprUnreachable, kExprElse, kExprUnreachable, kExprEnd];
......@@ -60,8 +53,6 @@ let loop_unr = [kExprLoop, kWasmVoid, kExprUnreachable, kExprEnd];
let block_block_unr = [kExprBlock, kWasmVoid, kExprBlock, kWasmVoid, kExprUnreachable, kExprEnd, kExprEnd];
let block = [kExprBlock, kWasmVoid]
let iblock = [kExprBlock, kWasmI32]
let i_iblock = [kExprBlock, 0]
let i_iiblock = [kExprBlock, 1]
let fblock = [kExprBlock, kWasmF32]
let end = kExprEnd;
let drop = kExprDrop;
......@@ -101,10 +92,6 @@ run(I, '(block U) iadd drop', [...block_unr, iadd, drop]);
run(I, '(block (block U)) iadd drop', [...block_block_unr, iadd, drop]);
run(I, '(loop U) iadd drop', [...loop_unr, iadd]);
run(I, '(if 0 U U) iadd drop', [...zero, ...if_else_unr, iadd, drop]);
run(I, 'U (i_iblock leqz)', [unr, ...i_iblock, leqz, end, drop]);
run(V, 'U (i_iblock ieqz)', [unr, ...i_iblock, ieqz, end, drop]);
run(I, 'U (iblock iadd)', [unr, ...iblock, iadd, end, drop]);
run(I, 'U zero64 (i_iiblock iadd) drop', [unr, ...zero64, ...i_iiblock, iadd, end, drop])
run(V, 'U 0 0 iadd drop', [unr, ...zero, ...zero, iadd, drop]);
run(V, "(block U) 0 0 iadd drop", [...block_unr, ...zero, ...zero, iadd, drop]);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment