[Mlir-commits] [mlir] [mlir] Own the registrations of SourceMgrDiagnosticVerifierHandler (PR #226258)
Henrich Lauko
llvmlistbot at llvm.org
Fri Sep 25 01:38:47 PDT 2026
https://github.com/xlauko updated https://github.com/llvm/llvm-project/pull/226258
>From 250f28b79b4dd16f41de305abbb5732fe0dd58be 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. performActions now finalizes the
remark policy on every exit instead of only after textual output, so
remarks deferred by --remark-policy=final reach the handlers registered
by the caller (the verifier, or the printing handler) when the pipeline
fails, rather than being emitted from ~MLIRContext after those handlers
are gone.
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 | 9 +--
mlir/lib/IR/Diagnostics.cpp | 24 +++----
mlir/lib/Tools/mlir-opt/MlirOptMain.cpp | 17 ++---
.../Pass/remark-final-verify-diagnostics.mlir | 10 +++
mlir/unittests/IR/Diagnostic.cpp | 62 +++++++++++++++++++
5 files changed, 99 insertions(+), 23 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 3b8fb46b06a484..0c02007251a0ec 100644
--- a/mlir/include/mlir/IR/Diagnostics.h
+++ b/mlir/include/mlir/IR/Diagnostics.h
@@ -652,12 +652,13 @@ 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:
- /// Process a single diagnostic.
+ /// Process a diagnostic and its notes.
void process(Diagnostic &diag);
/// Process a LocationAttr diagnostic.
diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index 5caf826c84bdd0..b6ebfa73c53027 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -833,7 +833,10 @@ SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler(
for (unsigned i = 0, e = mgr.getNumBuffers(); i != e; ++i)
(void)impl->computeExpectedDiags(out, mgr, mgr.getMemoryBuffer(i + 1));
- registerInContext(ctx);
+ // The base class registered a handler that prints every diagnostic. The
+ // verifier takes its place: it consumes diagnostics and reports unexpected
+ // ones itself.
+ setHandler([this](Diagnostic &diag) { process(diag); });
}
SourceMgrDiagnosticVerifierHandler::SourceMgrDiagnosticVerifierHandler(
@@ -866,20 +869,17 @@ LogicalResult SourceMgrDiagnosticVerifierHandler::verify() {
return impl->status;
}
-void SourceMgrDiagnosticVerifierHandler::registerInContext(MLIRContext *ctx) {
- ctx->getDiagEngine().registerHandler([&](Diagnostic &diag) {
- // Process the main diagnostics.
- process(diag);
-
- // Process each of the notes.
- for (auto ¬e : diag.getNotes())
- process(note);
- });
+std::unique_ptr<ScopedDiagnosticHandler>
+SourceMgrDiagnosticVerifierHandler::registerInContext(MLIRContext *ctx) {
+ return std::make_unique<ScopedDiagnosticHandler>(
+ ctx, [this](Diagnostic &diag) { process(diag); });
}
-/// Process a single diagnostic.
+/// Process a diagnostic and its notes.
void SourceMgrDiagnosticVerifierHandler::process(Diagnostic &diag) {
- return process(diag.getLocation(), diag.str(), diag.getSeverity());
+ process(diag.getLocation(), diag.str(), diag.getSeverity());
+ for (auto ¬e : diag.getNotes())
+ process(note);
}
/// Process a diagnostic at a certain location.
diff --git a/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp b/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp
index 8b49258e135d28..d4812aec50fc19 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"
@@ -580,6 +581,12 @@ performActions(raw_ostream &os,
break;
}
}
+ // Emit the remarks the policy deferred on every exit, while the caller's
+ // diagnostic handlers are still registered; ~MLIRContext would be too late.
+ llvm::scope_exit finalizeRemarks([&ctx] {
+ if (remark::detail::RemarkEngine *engine = ctx.getRemarkEngine())
+ engine->getRemarkEmittingPolicy()->finalize();
+ });
// Prepare the pass manager, applying command-line and reproducer options.
PassManager pm(op.get()->getName(), PassManager::Nesting::Implicit);
@@ -629,12 +636,6 @@ performActions(raw_ostream &os,
config.shouldVerifyPasses() && !pm.empty()),
/*locationMap=*/nullptr, &fallbackResourceMap);
os << OpWithState(op.get(), asmState) << '\n';
-
- // This is required if the remark policy is final. Otherwise, the remarks are
- // not emitted.
- if (remark::detail::RemarkEngine *engine = ctx.getRemarkEngine())
- engine->getRemarkEmittingPolicy()->finalize();
-
return success();
}
@@ -661,8 +662,10 @@ 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);
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 00000000000000..5e6ef7103ed5db
--- /dev/null
+++ b/mlir/test/Pass/remark-final-verify-diagnostics.mlir
@@ -0,0 +1,10 @@
+// RUN: mlir-opt %s --verify-diagnostics --remarks-filter="category-1-passed" \
+// RUN: --remark-policy=final \
+// RUN: --pass-pipeline='builtin.module(test-remark,test-pass-failure)'
+
+// The failing pipeline returns before the textual output; the remark deferred
+// by the final policy must still reach the diagnostic verifier.
+module {
+ // expected-remark at below {{[Passed] test-remark | Category:category-1-passed}}
+ "test.op"() : () -> ()
+}
diff --git a/mlir/unittests/IR/Diagnostic.cpp b/mlir/unittests/IR/Diagnostic.cpp
index 96e09d33309263..6390cdfe85acca 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