spirv-opt: Handle ID overflow in AggressiveDCEPass (#6366)
Refactored AggressiveDCEPass to return Pass::Status instead of bool
in several key functions. This allows the pass to gracefully fail
and propagate the error when the module's ID bound is reached,
preventing crashes due to null pointers or zero IDs.
Part of https://github.com/KhronosGroup/SPIRV-Tools/issues/6260
diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index df19294..795f8c3 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/source/opt/aggressive_dead_code_elim_pass.cpp
@@ -273,29 +273,30 @@
});
}
-bool AggressiveDCEPass::AggressiveDCE(Function* func) {
- if (func->IsDeclaration()) return false;
+Pass::Status AggressiveDCEPass::AggressiveDCE(Function* func) {
+ if (func->IsDeclaration()) return Pass::Status::SuccessWithoutChange;
std::list<BasicBlock*> structured_order;
cfg()->ComputeStructuredOrder(func, &*func->begin(), &structured_order);
live_local_vars_.clear();
InitializeWorkList(func, structured_order);
ProcessWorkList(func);
- ProcessDebugInformation(structured_order);
+ if (ProcessDebugInformation(structured_order) == Pass::Status::Failure)
+ return Pass::Status::Failure;
ProcessWorkList(func);
return KillDeadInstructions(func, structured_order);
}
-void AggressiveDCEPass::ProcessDebugInformation(
+Pass::Status AggressiveDCEPass::ProcessDebugInformation(
std::list<BasicBlock*>& structured_order) {
for (auto bi = structured_order.begin(); bi != structured_order.end(); bi++) {
- (*bi)->ForEachInst([this](Instruction* inst) {
+ bool succeeded = (*bi)->WhileEachInst([this](Instruction* inst) {
// DebugDeclare is not dead. It must be converted to DebugValue in a
// later pass
if (inst->IsNonSemanticInstruction() &&
inst->GetShader100DebugOpcode() ==
NonSemanticShaderDebugInfo100DebugDeclare) {
AddToWorklist(inst);
- return;
+ return true;
}
// If the Value of a DebugValue is killed, set Value operand to Undef
@@ -307,6 +308,9 @@
if (!live_insts_.Set(def->unique_id())) {
AddToWorklist(inst);
uint32_t undef_id = Type2Undef(def->type_id());
+ if (undef_id == 0) {
+ return false;
+ }
inst->SetInOperand(kDebugValueValue, {undef_id});
context()->get_def_use_mgr()->UpdateDefUse(inst);
id = inst->GetSingleWordInOperand(kDebugValueLocalVariable);
@@ -318,14 +322,18 @@
auto expression = get_def_use_mgr()->GetDef(id);
AddToWorklist(expression);
context()->get_def_use_mgr()->UpdateDefUse(expression);
- return;
+ return true;
}
}
+ return true;
});
+
+ if (!succeeded) return Pass::Status::Failure;
}
+ return Pass::Status::SuccessWithoutChange;
}
-bool AggressiveDCEPass::KillDeadInstructions(
+Pass::Status AggressiveDCEPass::KillDeadInstructions(
const Function* func, std::list<BasicBlock*>& structured_order) {
bool modified = false;
for (auto bi = structured_order.begin(); bi != structured_order.end();) {
@@ -360,6 +368,9 @@
// Find an undef for the return value and make sure it gets kept by
// the pass.
auto undef_id = Type2Undef(func->type_id());
+ if (undef_id == 0) {
+ return Pass::Status::Failure;
+ }
auto undef = get_def_use_mgr()->GetDef(undef_id);
live_insts_.Set(undef->unique_id());
merge_terminator->SetOpcode(spv::Op::OpReturnValue);
@@ -378,7 +389,8 @@
++bi;
}
}
- return modified;
+ return modified ? Pass::Status::SuccessWithChange
+ : Pass::Status::SuccessWithoutChange;
}
void AggressiveDCEPass::ProcessWorkList(Function* func) {
@@ -638,7 +650,7 @@
}
}
-void AggressiveDCEPass::InitializeModuleScopeLiveInstructions() {
+Pass::Status AggressiveDCEPass::InitializeModuleScopeLiveInstructions() {
// Keep all execution modes.
for (auto& exec : get_module()->execution_modes()) {
AddToWorklist(&exec);
@@ -713,6 +725,9 @@
}
if (debug_global_seen) {
auto dbg_none = context()->get_debug_info_mgr()->GetDebugInfoNone();
+ if (dbg_none == nullptr) {
+ return Pass::Status::Failure;
+ }
AddToWorklist(dbg_none);
}
@@ -726,6 +741,7 @@
AddToWorklist(&dbg);
}
}
+ return Pass::Status::SuccessWithoutChange;
}
Pass::Status AggressiveDCEPass::ProcessImpl() {
@@ -753,7 +769,9 @@
// Eliminate Dead functions.
bool modified = EliminateDeadFunctions();
- InitializeModuleScopeLiveInstructions();
+ if (InitializeModuleScopeLiveInstructions() == Pass::Status::Failure) {
+ return Pass::Status::Failure;
+ }
// Run |AggressiveDCE| on the remaining functions. The order does not matter,
// since |AggressiveDCE| is intra-procedural. This can mean that function
@@ -761,7 +779,13 @@
// function will still be in the module after this pass. We expect this to be
// rare.
for (Function& fp : *context()->module()) {
- modified |= AggressiveDCE(&fp);
+ Pass::Status function_status = AggressiveDCE(&fp);
+ if (function_status == Pass::Status::Failure) {
+ return Pass::Status::Failure;
+ }
+ if (function_status == Pass::Status::SuccessWithChange) {
+ modified = true;
+ }
}
// If the decoration manager is kept live then the context will try to keep it
diff --git a/source/opt/aggressive_dead_code_elim_pass.h b/source/opt/aggressive_dead_code_elim_pass.h
index c898d91..12a5ff7 100644
--- a/source/opt/aggressive_dead_code_elim_pass.h
+++ b/source/opt/aggressive_dead_code_elim_pass.h
@@ -85,8 +85,10 @@
}
// Adds entry points, execution modes and workgroup size decorations to the
- // worklist for processing with the first function.
- void InitializeModuleScopeLiveInstructions();
+ // worklist for processing with the first function. Returns
+ // Pass::Status::Failure if it could not create a required debug instruction.
+ // Returns Pass::Status::SuccessWithoutChange otherwise.
+ Pass::Status InitializeModuleScopeLiveInstructions();
// Add |inst| to worklist_ and live_insts_.
void AddToWorklist(Instruction* inst) {
@@ -136,7 +138,7 @@
// existing control structures will remain. This can leave not-insignificant
// sequences of ultimately useless code.
// TODO(): Remove useless control constructs.
- bool AggressiveDCE(Function* func);
+ Pass::Status AggressiveDCE(Function* func);
Pass::Status ProcessImpl();
@@ -154,11 +156,17 @@
// marked as live in the work list. DebugDeclare's are marked live now, and
// DebugValue Value operands are set to OpUndef. The work list will be empty
// at the end.
- void ProcessDebugInformation(std::list<BasicBlock*>& structured_order);
+ // Returns Pass::Status::Failure if it could not create an OpUndef.
+ // Otherwise, returns Pass::Status::SuccessWithChange if it made changes,
+ Pass::Status ProcessDebugInformation(
+ std::list<BasicBlock*>& structured_order);
// Kills any instructions in |func| that have not been marked as live.
- bool KillDeadInstructions(const Function* func,
- std::list<BasicBlock*>& structured_order);
+ // Returns Pass::Status::Failure if it could not create an OpUndef.
+ // Otherwise, returns Pass::Status::SuccessWithChange if it made changes,
+ // and Pass::Status::SuccessWithoutChange otherwise.
+ Pass::Status KillDeadInstructions(const Function* func,
+ std::list<BasicBlock*>& structured_order);
// Adds the instructions that define the operands of |inst| to the work list.
void AddOperandsToWorkList(const Instruction* inst);
diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp
index cc309da..c084a6c 100644
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@@ -390,6 +390,7 @@
if (debug_info_none_inst_ != nullptr) return debug_info_none_inst_;
uint32_t result_id = context()->TakeNextId();
+ if (result_id == 0) return nullptr;
std::unique_ptr<Instruction> dbg_info_none_inst(new Instruction(
context(), spv::Op::OpExtInst, context()->get_type_mgr()->GetVoidTypeId(),
result_id,
