[Mlir-commits] [mlir] [mlir] Make SourceMgrDiagnosticVerifierHandler callbacks inert after destruction (PR #226258)
Henrich Lauko
llvmlistbot at llvm.org
Thu Sep 24 14:12:03 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/226258
>From be8510b2b4e396ef7cd72ce74db8c9bdda718d52 Mon Sep 17 00:00:00 2001
From: Henrich Lauko <hlauko at nvidia.com>
Date: Wed, 23 Sep 2026 13:40:06 +0000
Subject: [PATCH] [mlir] Own the registrations of
SourceMgrDiagnosticVerifierHandler
registerInContext registered a callback that captured `this` and dropped
the handler id, so nothing could erase it; the constructor did the same
in the verifier's own context. The callbacks stayed registered after the
verifier was destroyed and later diagnostics reached the destroyed
handler. mlir-opt's per-buffer contexts die first, which hid the bug.
Install the constructor's callback through setHandler, so the base class
erases it, and return a scoped registration from registerInContext.
Callers destroy the handle before the verifier and the context; ignoring
the nodiscard result erases the registration immediately. Erasing from
every registered context in the destructor would not work: mlir-opt's
per-buffer contexts are gone by then.
mlir-opt keeps the handle per buffer and destroys the remark engine
before it, so remarks deferred by --remark-policy=final still reach the
verifier when the pass pipeline fails.
Add unit tests for the registration lifetimes and a lit test for the
deferred remark verification.
Co-Authored-By: Claude Fable 5.1 <noreply at anthropic.com>
---
mlir/include/mlir/IR/Diagnostics.h | 10 ++-
mlir/lib/IR/Diagnostics.cpp | 19 +++---
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp | 12 +++-
.../Pass/remark-final-verify-diagnostics.mlir | 14 +++++
mlir/unittests/IR/Diagnostic.cpp | 62 +++++++++++++++++++
5 files changed, 106 insertions(+), 11 deletions(-)
create mode 100644 mlir/test/Pass/remark-final-verify-diagnostics.mlir
diff --git a/mlir/include/mlir/IR/Diagnostics.h b/mlir/include/mlir/IR/Diagnostics.h
index 3b8fb46b06a48..bd93b26dad4a0 100644
--- a/mlir/include/mlir/IR/Diagnostics.h
+++ b/mlir/include/mlir/IR/Diagnostics.h
@@ -652,11 +652,15 @@ class SourceMgrDiagnosticVerifierHandler : public SourceMgrDiagnosticHandler {
/// verified correctly, failure otherwise.
LogicalResult verify();
- /// Register this handler with the given context. This is intended for use
- /// with the splitAndProcessBuffer function.
- void registerInContext(MLIRContext *ctx);
+ /// Register this handler with `ctx` and return a scoped registration.
+ /// Destroy the returned handle before either this handler or `ctx`.
+ [[nodiscard]] std::unique_ptr<ScopedDiagnosticHandler>
+ registerInContext(MLIRContext *ctx);
private:
+ /// Create a callback that processes a diagnostic and its notes.
+ DiagnosticEngine::HandlerTy makeHandler();
+
/// Process a single diagnostic.
void process(Diagnostic &diag);
diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index 5caf826c84bdd..32d6abe1259c9 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -833,7 +833,9 @@ SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler(
for (unsigned i = 0, e = mgr.getNumBuffers(); i != e; ++i)
(void)impl->computeExpectedDiags(out, mgr, mgr.getMemoryBuffer(i + 1));
- registerInContext(ctx);
+ // Replace the base class's printing handler; process() prints unexpected
+ // diagnostics.
+ setHandler(makeHandler());
}
SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler(
@@ -866,15 +868,18 @@ LogicalResult SourceMgrDiagnosticVerifierHandler::verify() {
return impl->status;
}
-void SourceMgrDiagnosticVerifierHandler::registerInContext(MLIRContext *ctx) {
- ctx->getDiagEngine().registerHandler([&](Diagnostic &diag) {
- // Process the main diagnostics.
+DiagnosticEngine::HandlerTy SourceMgrDiagnosticVerifierHandler::makeHandler() {
+ return [this](Diagnostic &diag) {
process(diag);
-
- // Process each of the notes.
for (auto ¬e : diag.getNotes())
process(note);
- });
+ return success();
+ };
+}
+
+std::unique_ptr<ScopedDiagnosticHandler>
+SourceMgrDiagnosticVerifierHandler::registerInContext(MLIRContext *ctx) {
+ return std::make_unique<ScopedDiagnosticHandler>(ctx, makeHandler());
}
/// Process a single diagnostic.
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index 8b49258e135d2..66267461297c8 100644
--- a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
+++ b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
@@ -34,6 +34,7 @@
#include "mlir/Tools/ParseUtilities.h"
#include "mlir/Tools/Plugins/DialectPlugin.h"
#include "mlir/Tools/Plugins/PassPlugin.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Remarks/RemarkFormat.h"
#include "llvm/Support/CommandLine.h"
@@ -661,8 +662,17 @@ processBuffer(raw_ostream &os, std::unique_ptr<MemoryBuffer> ownedBuffer,
MLIRContext context(registry, MLIRContext::Threading::DISABLED);
if (threadPool)
context.setThreadPool(*threadPool);
+ // Keep the registration within the lifetimes of the context and verifier.
+ std::unique_ptr<ScopedDiagnosticHandler> verifierRegistration;
if (verifyHandler)
- verifyHandler->registerInContext(&context);
+ verifierRegistration = verifyHandler->registerInContext(&context);
+
+ // Destroy the remark engine before erasing the verifier registration so
+ // deferred remarks are verified even when the pass pipeline fails.
+ llvm::scope_exit resetRemarkEngine([&] {
+ if (verifyHandler)
+ context.setRemarkEngine(nullptr);
+ });
StringRef irdlFile = config.getIrdlFile();
if (!irdlFile.empty() && failed(loadIRDLDialects(irdlFile, context)))
diff --git a/mlir/test/Pass/remark-final-verify-diagnostics.mlir b/mlir/test/Pass/remark-final-verify-diagnostics.mlir
new file mode 100644
index 0000000000000..8ba0e44ff0f1f
--- /dev/null
+++ b/mlir/test/Pass/remark-final-verify-diagnostics.mlir
@@ -0,0 +1,14 @@
+// RUN: mlir-opt %s --verify-diagnostics --remarks-filter="category.*" \
+// RUN: --remark-policy=final \
+// RUN: --pass-pipeline='builtin.module(test-remark,test-pass-failure)'
+
+// Fail before explicit remark finalization to check that remarks emitted
+// during cleanup still reach the diagnostic verifier.
+module {
+ // expected-remark at below {{Category:category-1-passed}}
+ // expected-remark at below {{Category:category-2-failed}}
+ // expected-remark at below {{Category:category-2-analysis}}
+ // expected-remark at below {{[Passed] test-remark | Category:category-link}}
+ // expected-remark at below {{[Analysis] test-remark | Category:category-link}}
+ "test.op"() : () -> ()
+}
diff --git a/mlir/unittests/IR/Diagnostic.cpp b/mlir/unittests/IR/Diagnostic.cpp
index 96e09d3330926..6390cdfe85acc 100644
--- a/mlir/unittests/IR/Diagnostic.cpp
+++ b/mlir/unittests/IR/Diagnostic.cpp
@@ -8,8 +8,12 @@
#include "mlir/IR/Diagnostics.h"
#include "mlir/Support/TypeID.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/raw_ostream.h"
#include "gtest/gtest.h"
+#include <memory>
+
using namespace mlir;
using namespace mlir::detail;
@@ -60,4 +64,62 @@ TEST(DiagnosticLifetime, TestLazyCopyStringLiteral) {
ASSERT_STREQ("^rror 1, mutate this", resultMessage.c_str());
}
+// Register counting handlers first so they receive diagnostics only after
+// the verifier's registration is erased.
+
+TEST(SourceMgrDiagnosticVerifierHandler, ScopedRegistration) {
+ MLIRContext own, other;
+ unsigned seen = 0;
+ ScopedDiagnosticHandler counter(&other, [&](Diagnostic &) { ++seen; });
+ llvm::SourceMgr sourceMgr;
+ llvm::raw_null_ostream out;
+ SourceMgrDiagnosticVerifierHandler verifier(
+ sourceMgr, &own, out,
+ SourceMgrDiagnosticVerifierHandler::Level::OnlyExpected);
+ {
+ std::unique_ptr<ScopedDiagnosticHandler> registration =
+ verifier.registerInContext(&other);
+ emitRemark(UnknownLoc::get(&other), "while registered");
+ EXPECT_EQ(seen, 0u);
+ }
+ emitRemark(UnknownLoc::get(&other), "after the handle");
+ EXPECT_EQ(seen, 1u);
+ EXPECT_TRUE(succeeded(verifier.verify()));
+}
+
+TEST(SourceMgrDiagnosticVerifierHandler, ErasesOwnRegistration) {
+ MLIRContext own;
+ unsigned seen = 0;
+ ScopedDiagnosticHandler counter(&own, [&](Diagnostic &) { ++seen; });
+ {
+ llvm::SourceMgr sourceMgr;
+ llvm::raw_null_ostream out;
+ SourceMgrDiagnosticVerifierHandler verifier(
+ sourceMgr, &own, out,
+ SourceMgrDiagnosticVerifierHandler::Level::OnlyExpected);
+ emitRemark(UnknownLoc::get(&own), "while alive");
+ EXPECT_EQ(seen, 0u);
+ }
+ emitRemark(UnknownLoc::get(&own), "after the verifier");
+ EXPECT_EQ(seen, 1u);
+}
+
+TEST(SourceMgrDiagnosticVerifierHandler, OutlivesRegisteredContext) {
+ // Match mlir-opt's per-buffer context lifetime.
+ MLIRContext context;
+ llvm::SourceMgr sourceMgr;
+ llvm::raw_null_ostream out;
+ SourceMgrDiagnosticVerifierHandler verifier(
+ sourceMgr, &context, out,
+ SourceMgrDiagnosticVerifierHandler::Level::OnlyExpected);
+ {
+ MLIRContext perBuffer;
+ std::unique_ptr<ScopedDiagnosticHandler> registration =
+ verifier.registerInContext(&perBuffer);
+ emitRemark(UnknownLoc::get(&perBuffer), "in the temporary context");
+ }
+ emitRemark(UnknownLoc::get(&context), "verifier still usable");
+ EXPECT_TRUE(succeeded(verifier.verify()));
+}
+
} // namespace
More information about the Mlir-commits
mailing list