[Mlir-commits] [mlir] [mlir] Fix assertion crash (#159673) (PR #173020)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-core

Author: Jelle Schühmacher (jschuhmacher)

<details>
<summary>Changes</summary>

MLIR crashes with assertion 'inserted.second && "expected region to contain uniquely named symbol operations"' failed when constructing a symbol table from another symbol table that has duplicate symbols.

The API allowed a SymbolTable instance to be in this state in the first place, I think copying it should be allowed in that state as well. Simply removing the assert allows construction of the instance, and allows a subsequent run of the verifier to report the actual error.

---
Full diff: https://github.com/llvm/llvm-project/pull/173020.diff


2 Files Affected:

- (modified) mlir/lib/IR/SymbolTable.cpp (-2) 
- (modified) mlir/unittests/IR/SymbolTableTest.cpp (+30) 


``````````diff
diff --git a/mlir/lib/IR/SymbolTable.cpp b/mlir/lib/IR/SymbolTable.cpp
index 9f5dd2c9e3b72..528b4f586d66f 100644
--- a/mlir/lib/IR/SymbolTable.cpp
+++ b/mlir/lib/IR/SymbolTable.cpp
@@ -132,8 +132,6 @@ SymbolTable::SymbolTable(Operation *symbolTableOp)
 
     auto inserted = symbolTable.insert({name, &op});
     (void)inserted;
-    assert(inserted.second &&
-           "expected region to contain uniquely named symbol operations");
   }
 }
 
diff --git a/mlir/unittests/IR/SymbolTableTest.cpp b/mlir/unittests/IR/SymbolTableTest.cpp
index 864eb40898335..45d48f62c10b3 100644
--- a/mlir/unittests/IR/SymbolTableTest.cpp
+++ b/mlir/unittests/IR/SymbolTableTest.cpp
@@ -166,4 +166,34 @@ TEST(SymbolOpInterface, Visibility) {
   ASSERT_EQ(diagStr, expectedDiag);
 }
 
+TEST(SymbolOpInterface, Duplicate) {
+  DialectRegistry registry;
+  ::test::registerTestDialect(registry);
+  MLIRContext context(registry);
+
+  constexpr static StringLiteral kInput = R"MLIR(
+    "test.overridden_symbol_visibility"() {sym_name = "duplicate_symbol_name"} : () -> ()
+    "test.overridden_symbol_visibility"() {sym_name = "duplicate_symbol_name"} : () -> ()
+  )MLIR";
+
+  constexpr bool verifyAfterParse = false;
+  ParserConfig parserConfig(&context, verifyAfterParse);
+  OwningOpRef<ModuleOp> module =
+      parseSourceString<ModuleOp>(kInput, parserConfig);
+
+  // copying an existing symboltable instance should not crash
+  SymbolTable symbolTable(module.get());
+
+  std::string diagStr;
+  constexpr static StringLiteral expectedDiag =
+      "redefinition of symbol named 'duplicate_symbol_name'";
+  context.getDiagEngine().registerHandler(
+      [&diagStr](Diagnostic &diag) { diagStr += diag.str(); });
+
+  LogicalResult res = mlir::verify(module.get(), true);
+
+  ASSERT_FALSE(succeeded(res));
+  ASSERT_EQ(diagStr, expectedDiag);
+}
+
 } // namespace

``````````

</details>


https://github.com/llvm/llvm-project/pull/173020