Commit 6023bfa7 authored by Leszek Swirski's avatar Leszek Swirski Committed by V8 LUCI CQ

[maglev] Fix flaky crash around block merging

The is_unmerged_loop predicate was using the "unmerged_loop_marker"
predecessor sentinel value to decide whether the merge state is an
unmerged loop header or not. However, the predecessor values were
otherwise uninitialized. This means that with some amount of bad luck,
you could get an uninitialized predecessor which happened to hold the
unmerged loop marker (it's more likely than a 1 in 2^32 chance, because
it could be left over from a previous compilation's zone).

Since we anyway now store whether a merge state is a loop header for
other reasons, we can replace the sentinel logic with predecessor count
based logic for this predicate.

Bug: v8:7700, v8:13109
Change-Id: Ibabe23feefc2bb909cf2480113300cb4757114d3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3807591
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: 's avatarVictor Gomes <victorgomes@chromium.org>
Commit-Queue: Victor Gomes <victorgomes@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82166}
parent 13ecd2c3
......@@ -285,8 +285,6 @@ class MergePointInterpreterFrameState {
}
});
DCHECK(!frame_state_.liveness()->AccumulatorIsLive());
predecessors_[predecessor_count - 1] = unmerged_loop_marker();
}
// Merges an unmerged framestate with a possibly merged framestate into |this|
......@@ -368,10 +366,6 @@ class MergePointInterpreterFrameState {
int merge_offset) {
DCHECK_GT(predecessor_count_, 1);
DCHECK_LT(predecessors_so_far_, predecessor_count_);
// For loops, we need to pull the unmerged loop marker back by one.
if (is_unmerged_loop()) {
predecessors_[predecessor_count_ - 2] = unmerged_loop_marker();
}
predecessor_count_--;
DCHECK_LE(predecessors_so_far_, predecessor_count_);
......@@ -419,15 +413,17 @@ class MergePointInterpreterFrameState {
bool is_loop() const { return is_loop_header_; }
bool is_unmerged_loop() const {
// If this is a loop and not all predecessors are set, then the loop isn't
// merged yet.
DCHECK_GT(predecessor_count_, 0);
return predecessors_[predecessor_count_ - 1] == unmerged_loop_marker();
return is_loop_header_ && predecessors_so_far_ < predecessor_count_;
}
bool is_unreachable_loop() const {
// If there is only one predecessor, and it's not set, then this is a loop
// merge with no forward control flow entering it.
return predecessor_count_ == 1 &&
predecessors_[0] == unmerged_loop_marker();
return is_loop_header_ && predecessor_count_ == 1 &&
predecessors_so_far_ == 0;
}
private:
......@@ -435,11 +431,6 @@ class MergePointInterpreterFrameState {
const MaglevCompilationUnit& info,
const MergePointInterpreterFrameState& state);
// Create an uninitialized value sentinel for loop merges.
static BasicBlock* unmerged_loop_marker() {
return reinterpret_cast<BasicBlock*>(0x100b);
}
ValueNode* FromInt32ToTagged(MaglevCompilationUnit& compilation_unit,
ValueNode* value) {
DCHECK_EQ(value->properties().value_representation(),
......
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