[MLIR][SymbolTable] Fix crash when SymbolTable is built on unverified IR#183945
Merged
Conversation
The SymbolTable::SymbolTable constructor asserted that all symbol names in the region were unique. This could cause mlir-opt to crash instead of producing a proper diagnostic when the IR contained both: 1. An IsolatedFromAbove op (e.g., irdl.dialect) with a symbol user that looks up symbols in an ancestor symbol table, and 2. Duplicate symbols in that ancestor (e.g., two func.func @test). The crash occurred because IsolatedFromAbove ops are verified in verifyOnExit() before verifyRegionInvariants() runs the SymbolTable trait's verifyRegionTrait (which produces the proper duplicate-symbol diagnostic). When the isolated op's symbol use verification triggered SymbolTableCollection::getSymbolTable() on the ancestor, the constructor would assert instead of gracefully handling the invalid-but-not-yet- reported duplicate symbols. The fix removes the assertion and silently skips duplicate symbol insertions (keeping the first definition). The proper diagnostic is still produced by the SymbolTable trait's verifyRegionTrait, which runs after all children have been verified. Add a regression test in invalid.irdl.mlir using a self-referencing IRDL parametric type combined with duplicate function symbols. Fixes llvm#159673
Member
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Mehdi Amini (joker-eph) ChangesThe SymbolTable::SymbolTable constructor asserted that all symbol names in the region were unique. This could cause mlir-opt to crash instead of producing a proper diagnostic when the IR contained both:
The crash occurred because IsolatedFromAbove ops are verified in verifyOnExit() before verifyRegionInvariants() runs the SymbolTable trait's verifyRegionTrait (which produces the proper duplicate-symbol diagnostic). When the isolated op's symbol use verification triggered SymbolTableCollection::getSymbolTable() on the ancestor, the constructor would assert instead of gracefully handling the invalid-but-not-yet- reported duplicate symbols. The fix removes the assertion and silently skips duplicate symbol insertions (keeping the first definition). The proper diagnostic is still produced by the SymbolTable trait's verifyRegionTrait, which runs after all children have been verified. Add a regression test in invalid.irdl.mlir using a self-referencing IRDL parametric type combined with duplicate function symbols. Fixes #159673 Full diff: https://github.com/llvm/llvm-project/pull/183945.diff 2 Files Affected:
diff --git a/mlir/lib/IR/SymbolTable.cpp b/mlir/lib/IR/SymbolTable.cpp
index 4e191e7d612ad..078401c8380f4 100644
--- a/mlir/lib/IR/SymbolTable.cpp
+++ b/mlir/lib/IR/SymbolTable.cpp
@@ -130,10 +130,13 @@ SymbolTable::SymbolTable(Operation *symbolTableOp)
if (!name)
continue;
- auto inserted = symbolTable.insert({name, &op});
- (void)inserted;
- assert(inserted.second &&
- "expected region to contain uniquely named symbol operations");
+ // Silently skip duplicate symbol names. Duplicate symbols are an
+ // invalid IR condition diagnosed by the SymbolTable trait's
+ // verifyRegionTrait. The constructor may be called before verification
+ // completes (e.g., when IsolatedFromAbove ops look up symbols in an
+ // ancestor symbol table during verification), so an assert here would
+ // crash instead of producing a proper diagnostic.
+ symbolTable.try_emplace(name, &op);
}
}
diff --git a/mlir/test/Dialect/IRDL/invalid.irdl.mlir b/mlir/test/Dialect/IRDL/invalid.irdl.mlir
index 8a7fffe1a9cbd..4ff7445c80de8 100644
--- a/mlir/test/Dialect/IRDL/invalid.irdl.mlir
+++ b/mlir/test/Dialect/IRDL/invalid.irdl.mlir
@@ -179,3 +179,23 @@ irdl.dialect @invalid_parametric {
irdl.results(foo: %param)
}
}
+
+// -----
+
+// Regression test for https://github.com/llvm/llvm-project/issues/159673
+// A self-referencing IRDL parametric type combined with duplicate function
+// symbols in the enclosing module used to crash with a SymbolTable assertion
+// instead of producing a proper "redefinition of symbol" diagnostic.
+
+irdl.dialect @testd {
+ irdl.type @self_referencing {
+ %0 = irdl.any
+ %1 = irdl.parametric @testd::@self_referencing<%0>
+ irdl.parameters(foo: %1)
+ }
+}
+
+// expected-note@+1 {{see existing symbol definition here}}
+func.func @test() { return }
+// expected-error@+1 {{redefinition of symbol named 'test'}}
+func.func @test() { return }
|
Moxinilian
approved these changes
Mar 1, 2026
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/18047 Here is the relevant piece of the build log for the reference |
sahas3
pushed a commit
to sahas3/llvm-project
that referenced
this pull request
Mar 4, 2026
… IR (llvm#183945) The SymbolTable::SymbolTable constructor asserted that all symbol names in the region were unique. This could cause mlir-opt to crash instead of producing a proper diagnostic when the IR contained both: 1. An IsolatedFromAbove op (e.g., irdl.dialect) with a symbol user that looks up symbols in an ancestor symbol table, and 2. Duplicate symbols in that ancestor (e.g., two func.func @test). The crash occurred because IsolatedFromAbove ops are verified in verifyOnExit() before verifyRegionInvariants() runs the SymbolTable trait's verifyRegionTrait (which produces the proper duplicate-symbol diagnostic). When the isolated op's symbol use verification triggered SymbolTableCollection::getSymbolTable() on the ancestor, the constructor would assert instead of gracefully handling the invalid-but-not-yet- reported duplicate symbols. The fix removes the assertion and silently skips duplicate symbol insertions (keeping the first definition). The proper diagnostic is still produced by the SymbolTable trait's verifyRegionTrait, which runs after all children have been verified. Add a regression test in invalid.irdl.mlir using a self-referencing IRDL parametric type combined with duplicate function symbols. Fixes llvm#159673
sujianIBM
pushed a commit
to sujianIBM/llvm-project
that referenced
this pull request
Mar 5, 2026
… IR (llvm#183945) The SymbolTable::SymbolTable constructor asserted that all symbol names in the region were unique. This could cause mlir-opt to crash instead of producing a proper diagnostic when the IR contained both: 1. An IsolatedFromAbove op (e.g., irdl.dialect) with a symbol user that looks up symbols in an ancestor symbol table, and 2. Duplicate symbols in that ancestor (e.g., two func.func @test). The crash occurred because IsolatedFromAbove ops are verified in verifyOnExit() before verifyRegionInvariants() runs the SymbolTable trait's verifyRegionTrait (which produces the proper duplicate-symbol diagnostic). When the isolated op's symbol use verification triggered SymbolTableCollection::getSymbolTable() on the ancestor, the constructor would assert instead of gracefully handling the invalid-but-not-yet- reported duplicate symbols. The fix removes the assertion and silently skips duplicate symbol insertions (keeping the first definition). The proper diagnostic is still produced by the SymbolTable trait's verifyRegionTrait, which runs after all children have been verified. Add a regression test in invalid.irdl.mlir using a self-referencing IRDL parametric type combined with duplicate function symbols. Fixes llvm#159673
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SymbolTable::SymbolTable constructor asserted that all symbol names in the region were unique. This could cause mlir-opt to crash instead of producing a proper diagnostic when the IR contained both:
The crash occurred because IsolatedFromAbove ops are verified in verifyOnExit() before verifyRegionInvariants() runs the SymbolTable trait's verifyRegionTrait (which produces the proper duplicate-symbol diagnostic). When the isolated op's symbol use verification triggered SymbolTableCollection::getSymbolTable() on the ancestor, the constructor would assert instead of gracefully handling the invalid-but-not-yet- reported duplicate symbols.
The fix removes the assertion and silently skips duplicate symbol insertions (keeping the first definition). The proper diagnostic is still produced by the SymbolTable trait's verifyRegionTrait, which runs after all children have been verified.
Add a regression test in invalid.irdl.mlir using a self-referencing IRDL parametric type combined with duplicate function symbols.
Fixes #159673