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

[maglev] Clear register state in exception handlers

Exception handlers were allowing register state to leak through, which
had knock-on effects of Phi allocation inserting gap moves in an illegal
location (specifically, at the end of the block, thinking that it's
allocating a control node since it's not allocating a body node).

Fix the register leak by clearing register state, and add some invariant
guards in the areas where the failure appeared.

Bug: v8:7700
Change-Id: I15c1fba1a250e295f0147a4e51a6c8c5481e8c7e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3890989Reviewed-by: 's avatarJakob Linke <jgruber@chromium.org>
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83129}
parent 1f529f2b
......@@ -298,6 +298,9 @@ constexpr bool IsConstantNode(Opcode opcode) {
constexpr bool IsGapMoveNode(Opcode opcode) {
return kFirstGapMoveNodeOpcode <= opcode && opcode <= kLastGapMoveNodeOpcode;
}
constexpr bool IsControlNode(Opcode opcode) {
return kFirstControlNodeOpcode <= opcode && opcode <= kLastControlNodeOpcode;
}
constexpr bool IsBranchControlNode(Opcode opcode) {
return kFirstBranchControlNodeOpcode <= opcode &&
opcode <= kLastBranchControlNodeOpcode;
......@@ -1007,6 +1010,10 @@ constexpr bool NodeBase::Is<ValueNode>() const {
return IsValueNode(opcode());
}
template <>
constexpr bool NodeBase::Is<ControlNode>() const {
return IsControlNode(opcode());
}
template <>
constexpr bool NodeBase::Is<BranchControlNode>() const {
return IsBranchControlNode(opcode());
}
......
......@@ -311,10 +311,16 @@ void StraightForwardRegisterAllocator::AllocateRegisters() {
for (block_it_ = graph_->begin(); block_it_ != graph_->end(); ++block_it_) {
BasicBlock* block = *block_it_;
current_node_ = nullptr;
// Restore mergepoint state.
if (block->has_state() && !block->state()->is_exception_handler()) {
InitializeRegisterValues(block->state()->register_state());
if (block->has_state()) {
if (block->state()->is_exception_handler()) {
// Exceptions start from a blank state of register values.
ClearRegisterValues();
} else {
InitializeRegisterValues(block->state()->register_state());
}
} else if (block->is_empty_block()) {
InitializeRegisterValues(block->empty_block_register_state());
}
......@@ -543,6 +549,11 @@ Register GetNodeResultRegister(Node* node) {
#endif // DEBUG
void StraightForwardRegisterAllocator::AllocateNode(Node* node) {
// We shouldn't be visiting any gap moves during allocation, we should only
// have inserted gap moves in past visits.
DCHECK(!node->Is<GapMove>());
DCHECK(!node->Is<ConstantGapMove>());
current_node_ = node;
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os()
......@@ -562,7 +573,6 @@ void StraightForwardRegisterAllocator::AllocateNode(Node* node) {
AllocateNodeResult(node->Cast<ValueNode>());
}
current_node_ = node;
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os() << "Updating uses...\n";
}
......@@ -570,12 +580,25 @@ void StraightForwardRegisterAllocator::AllocateNode(Node* node) {
// Update uses only after allocating the node result. This order is necessary
// to avoid emitting input-clobbering gap moves during node result allocation.
if (node->properties().can_eager_deopt()) {
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os() << "Using eager deopt nodes...\n";
}
UpdateUse(*node->eager_deopt_info());
}
for (Input& input : *node) UpdateUse(&input);
for (Input& input : *node) {
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os()
<< "Using input " << PrintNodeLabel(graph_labeller(), input.node())
<< "...\n";
}
UpdateUse(&input);
}
// Lazy deopts are semantically after the node, so update them last.
if (node->properties().can_lazy_deopt()) {
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os() << "Using lazy deopt nodes...\n";
}
UpdateUse(*node->lazy_deopt_info());
}
......@@ -933,6 +956,7 @@ void StraightForwardRegisterAllocator::AddMoveBeforeCurrentNode(
graph_labeller()->RegisterNode(gap_move);
}
if (*node_it_ == nullptr) {
DCHECK(current_node_->Is<ControlNode>());
// We're at the control node, so append instead.
(*block_it_)->nodes().Add(gap_move);
node_it_ = (*block_it_)->nodes().end();
......@@ -1519,15 +1543,19 @@ void StraightForwardRegisterAllocator::ForEachMergePointRegisterState(
});
}
void StraightForwardRegisterAllocator::InitializeRegisterValues(
MergePointRegisterState& target_state) {
// First clear the register state.
void StraightForwardRegisterAllocator::ClearRegisterValues() {
ClearRegisterState(general_registers_);
ClearRegisterState(double_registers_);
// All registers should be free by now.
DCHECK_EQ(general_registers_.unblocked_free(), kAllocatableGeneralRegisters);
DCHECK_EQ(double_registers_.unblocked_free(), kAllocatableDoubleRegisters);
}
void StraightForwardRegisterAllocator::InitializeRegisterValues(
MergePointRegisterState& target_state) {
// First clear the register state.
ClearRegisterValues();
// Then fill it in with target information.
auto fill = [&](auto& registers, auto reg, RegisterState& state) {
......
......@@ -203,6 +203,7 @@ class StraightForwardRegisterAllocator {
void ForEachMergePointRegisterState(
MergePointRegisterState& merge_point_state, Function&& f);
void ClearRegisterValues();
void InitializeRegisterValues(MergePointRegisterState& target_state);
#ifdef DEBUG
bool IsInRegister(MergePointRegisterState& target_state, ValueNode* incoming);
......
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