Skip to content

Commit

Permalink
[Backport] Security bug 1509576
Browse files Browse the repository at this point in the history
Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/v8/v8/+/5114883:
Merged: [turboshaft] Fix StructuralOptimization because of ignored side-effects

Side-effects in the 1st else block were not taken into account.

Drive-by: minor cleanups to StructuralOptimizationReducer.

Bug: v8:12783, chromium:1509576
(cherry picked from commit 4a664b390577de3d3572010da0dc1138d78ab2c4)

Change-Id: Id4e230ee0fd408c821747d3350d688c8b0098ae3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5114883
Reviewed-by: Matthias Liedtke <[email protected]>
Commit-Queue: Matthias Liedtke <[email protected]>
Auto-Submit: Darius Mercadier <[email protected]>
Cr-Commit-Position: refs/branch-heads/12.0@{#20}
Cr-Branched-From: ed7b4caf1fb8184ad9e24346c84424055d4d430a-refs/heads/12.0.267@{#1}
Cr-Branched-From: 210e75b19db4352c9b78dce0bae11c2dc3077df4-refs/heads/main@{#90651}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/530060
Reviewed-by: Allan Sandfeld Jensen <[email protected]>
  • Loading branch information
Darius M authored and mibrunin committed Jan 16, 2024
1 parent 17bc3bc commit dd0db25
Showing 1 changed file with 11 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ namespace v8::internal::compiler::turboshaft {
template <class Next>
class StructuralOptimizationReducer : public Next {
public:
using Next::Asm;
TURBOSHAFT_REDUCER_BOILERPLATE()

OpIndex ReduceInputGraphBranch(OpIndex input_index, const BranchOp& branch) {
LABEL_BLOCK(no_change) {
Expand All @@ -100,6 +100,13 @@ class StructuralOptimizationReducer : public Next {

OpIndex switch_var = OpIndex::Invalid();
while (true) {
// The "false" destination will be inlined before the switch is emitted,
// so it should only contain pure operations.
if (!ContainsOnlyPureOps(current_branch->if_false, Asm().input_graph())) {
TRACE("\t [break] End of only-pure-ops cascade reached.\n");
break;
}

// If we encounter a condition that is not equality, we can't turn it
// into a switch case.
const EqualOp* equal = Asm()
Expand All @@ -116,17 +123,11 @@ class StructuralOptimizationReducer : public Next {
// MachineOptimizationReducer should normalize equality to put constants
// right.
const Operation& right_op = Asm().input_graph().Get(equal->right());
if (!right_op.Is<ConstantOp>()) {
TRACE("\t [bailout] No constant on the right side of Equal.\n");
if (!right_op.Is<Opmask::kWord32Constant>()) {
TRACE("\t [bailout] No Word32 constant on the right side of Equal.\n");
break;
}

// We can only turn Word32 constant equals to switch cases.
const ConstantOp& const_op = right_op.Cast<ConstantOp>();
if (const_op.kind != ConstantOp::Kind::kWord32) {
TRACE("\t [bailout] Constant is not of type Word32.\n");
break;
}

// If we encounter equal to a different value, we can't introduce
// a switch.
Expand Down Expand Up @@ -164,13 +165,6 @@ class StructuralOptimizationReducer : public Next {

// Iterate to the next if_false block in the cascade.
current_branch = &maybe_branch.template Cast<BranchOp>();

// As long as the else blocks contain only pure ops, we can keep
// traversing the if-else cascade.
if (!ContainsOnlyPureOps(current_branch->if_false, Asm().input_graph())) {
TRACE("\t [break] End of only-pure-ops cascade reached.\n");
break;
}
}

// Probably better to keep short if-else cascades as they are.
Expand All @@ -186,7 +180,7 @@ class StructuralOptimizationReducer : public Next {
InlineAllOperationsWithoutLast(block);
}

TRACE("[reduce] Successfully emit a Switch with %z cases.", cases.size());
TRACE("[reduce] Successfully emit a Switch with %zu cases.", cases.size());

// The last current_if_true block that ends the cascade becomes the default
// case.
Expand Down

0 comments on commit dd0db25

Please sign in to comment.