Handle id overflow in sroa better. (#2582)
There is a case where sroa is not handling id overflow gracefully. It
is handled and an error message is output when the ids overflow.
Fixes https://crbug.com/961030.
diff --git a/source/opt/constants.cpp b/source/opt/constants.cpp
index 768364b..8dce8ec 100644
--- a/source/opt/constants.cpp
+++ b/source/opt/constants.cpp
@@ -167,6 +167,10 @@
const Constant* new_const, Module::inst_iterator* pos, uint32_t type_id) {
// TODO(1841): Handle id overflow.
uint32_t new_id = context()->TakeNextId();
+ if (new_id == 0) {
+ return nullptr;
+ }
+
auto new_inst = CreateInstruction(new_id, new_const, type_id);
if (!new_inst) {
return nullptr;
diff --git a/source/opt/ir_context.h b/source/opt/ir_context.h
index 185b494..c857c52 100644
--- a/source/opt/ir_context.h
+++ b/source/opt/ir_context.h
@@ -456,7 +456,16 @@
// Return the next available SSA id and increment it. Returns 0 if the
// maximum SSA id has been reached.
- inline uint32_t TakeNextId() { return module()->TakeNextIdBound(); }
+ inline uint32_t TakeNextId() {
+ uint32_t next_id = module()->TakeNextIdBound();
+ if (next_id == 0) {
+ if (consumer()) {
+ std::string message = "ID overflow. Try running compact-ids.";
+ consumer()(SPV_MSG_ERROR, "", {0, 0, 0}, message.c_str());
+ }
+ }
+ return next_id;
+ }
FeatureManager* get_feature_mgr() {
if (!feature_mgr_.get()) {
diff --git a/source/opt/scalar_replacement_pass.cpp b/source/opt/scalar_replacement_pass.cpp
index bfa8598..6997bf6 100644
--- a/source/opt/scalar_replacement_pass.cpp
+++ b/source/opt/scalar_replacement_pass.cpp
@@ -72,7 +72,9 @@
bool ScalarReplacementPass::ReplaceVariable(
Instruction* inst, std::queue<Instruction*>* worklist) {
std::vector<Instruction*> replacements;
- CreateReplacementVariables(inst, &replacements);
+ if (!CreateReplacementVariables(inst, &replacements)) {
+ return false;
+ }
std::vector<Instruction*> dead;
dead.push_back(inst);
@@ -257,7 +259,7 @@
return true;
}
-void ScalarReplacementPass::CreateReplacementVariables(
+bool ScalarReplacementPass::CreateReplacementVariables(
Instruction* inst, std::vector<Instruction*>* replacements) {
Instruction* type = GetStorageType(inst);
@@ -302,6 +304,8 @@
}
TransferAnnotations(inst, replacements);
+ return std::find(replacements->begin(), replacements->end(), nullptr) ==
+ replacements->end();
}
void ScalarReplacementPass::TransferAnnotations(
@@ -801,7 +805,9 @@
const analysis::Constant* null_const = const_mgr->GetConstant(type, {});
Instruction* null_inst =
const_mgr->GetDefiningInstruction(null_const, type_id);
- context()->UpdateDefUse(null_inst);
+ if (null_inst != nullptr) {
+ context()->UpdateDefUse(null_inst);
+ }
return null_inst;
}
diff --git a/source/opt/scalar_replacement_pass.h b/source/opt/scalar_replacement_pass.h
index 0ff9947..ca8c0b4 100644
--- a/source/opt/scalar_replacement_pass.h
+++ b/source/opt/scalar_replacement_pass.h
@@ -117,8 +117,8 @@
// for element of the composite type. Uses of |inst| are updated as
// appropriate. If the replacement variables are themselves scalarizable, they
// get added to |worklist| for further processing. If any replacement
- // variable ends up with no uses it is erased. Returns false if any
- // subsequent access chain is out of bounds.
+ // variable ends up with no uses it is erased. Returns false if the variable
+ // could not be replaced.
bool ReplaceVariable(Instruction* inst, std::queue<Instruction*>* worklist);
// Returns the underlying storage type for |inst|.
@@ -145,13 +145,14 @@
std::vector<Instruction*>* replacements);
// Populates |replacements| with a new OpVariable for each element of |inst|.
+ // Returns true if the replacement variables were successfully created.
//
// |inst| must be an OpVariable of a composite type. New variables are
// initialized the same as the corresponding index in |inst|. |replacements|
// will contain a variable for each element of the composite with matching
// indexes (i.e. the 0'th element of |inst| is the 0'th entry of
// |replacements|).
- void CreateReplacementVariables(Instruction* inst,
+ bool CreateReplacementVariables(Instruction* inst,
std::vector<Instruction*>* replacements);
// Returns the value of an OpConstant of integer type.
@@ -217,6 +218,7 @@
// Returns an instruction defining a null constant with type |type_id|. If
// one already exists, it is returned. Otherwise a new one is created.
+ // Returns |nullptr| if the new constant could not be created.
Instruction* CreateNullConstant(uint32_t type_id);
// Maps storage type to a pointer type enclosing that type.
diff --git a/test/opt/scalar_replacement_test.cpp b/test/opt/scalar_replacement_test.cpp
index a53f09d..6c182d5 100644
--- a/test/opt/scalar_replacement_test.cpp
+++ b/test/opt/scalar_replacement_test.cpp
@@ -1620,6 +1620,43 @@
EXPECT_EQ(Pass::Status::SuccessWithoutChange, std::get<1>(result));
}
+// Test that id overflow is handled gracefully.
+TEST_F(ScalarReplacementTest, IdBoundOverflow) {
+ const std::string text = R"(
+OpCapability ImageQuery
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %4 "main"
+OpExecutionMode %4 OriginUpperLeft
+OpDecorate %4194302 DescriptorSet 1073495039
+%2 = OpTypeVoid
+%3 = OpTypeFunction %2
+%6 = OpTypeFloat 32
+%7 = OpTypeStruct %6 %6
+%557056 = OpTypeStruct %7
+%9 = OpTypePointer Function %7
+%18 = OpTypeFunction %7 %9
+%4 = OpFunction %2 Pure|Const %3
+%1836763 = OpLabel
+%4194302 = OpVariable %9 Function
+%10 = OpVariable %9 Function
+OpKill
+%4194301 = OpLabel
+%524296 = OpLoad %7 %4194302
+OpKill
+OpFunctionEnd
+ )";
+
+ SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+
+ std::vector<Message> messages = {
+ {SPV_MSG_ERROR, "", 0, 0, "ID overflow. Try running compact-ids."},
+ {SPV_MSG_ERROR, "", 0, 0, "ID overflow. Try running compact-ids."}};
+ SetMessageConsumer(GetTestMessageConsumer(messages));
+ auto result =
+ SinglePassRunAndDisassemble<ScalarReplacementPass>(text, true, false);
+ EXPECT_EQ(Pass::Status::Failure, std::get<1>(result));
+}
+
} // namespace
} // namespace opt
} // namespace spvtools
