[Mlir-commits] [mlir] [mlir][Func] Fix FuncOp verifier ordering via hasRegionVerifier (PR #184612)

[Mehdi Amini]

https://github.com/joker-eph created https://github.com/llvm/llvm-project/pull/184612

FuncOp::verify() iterated over all blocks and called getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface terminator to check return types.  This ran during the entrance phase of verification — before child ops had been verified — so a malformed terminator whose getMutableSuccessorOperands() assumed invariants established by its own verify() could crash instead of emitting a clean diagnostic.

Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions() so the return-type checks run in the exit phase, after all nested ops have already been verified.

To demonstrate the bug and guard against regression, add TestCrashingReturnOp to the test dialect.  The op implements RegionBranchTerminatorOpInterface and report_fatal_errors in getMutableSuccessorOperands() when its 'valid' unit-attr is absent, reproducing the class of crash described above.  The accompanying lit test confirms a clean diagnostic is emitted rather than a crash.

>From a736dafda4a4b6dd49dac7b73802ba0195b261e9 Mon Sep 17 00:00:00 2001
From: Mehdi Amini <joker.eph at gmail.com>
Date: Wed, 4 Mar 2026 06:09:09 -0800
Subject: [PATCH] [mlir][Func] Fix FuncOp verifier ordering via
 hasRegionVerifier
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

FuncOp::verify() iterated over all blocks and called
getMutableSuccessorOperands() on any RegionBranchTerminatorOpInterface
terminator to check return types.  This ran during the entrance phase of
verification — before child ops had been verified — so a malformed
terminator whose getMutableSuccessorOperands() assumed invariants established
by its own verify() could crash instead of emitting a clean diagnostic.

Fix by switching to hasRegionVerifier=1: rename verify() → verifyRegions()
so the return-type checks run in the exit phase, after all nested ops have
already been verified.

To demonstrate the bug and guard against regression, add
TestCrashingReturnOp to the test dialect.  The op implements
RegionBranchTerminatorOpInterface and report_fatal_errors in
getMutableSuccessorOperands() when its 'valid' unit-attr is absent,
reproducing the class of crash described above.  The accompanying lit test
confirms a clean diagnostic is emitted rather than a crash.
---
 mlir/include/mlir/Dialect/Func/IR/FuncOps.td   |  2 +-
 mlir/lib/Dialect/Func/IR/FuncOps.cpp           |  2 +-
 .../Func/func-verifier-order-crash.mlir        | 12 ++++++++++++
 mlir/test/lib/Dialect/Test/TestOpDefs.cpp      | 18 ++++++++++++++++++
 mlir/test/lib/Dialect/Test/TestOps.td          | 11 +++++++++++
 5 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100644 mlir/test/Dialect/Func/func-verifier-order-crash.mlir

diff --git a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
index ce2b7228cb954..a31b860276099 100644
--- a/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
+++ b/mlir/include/mlir/Dialect/Func/IR/FuncOps.td
@@ -357,7 +357,7 @@ def FuncOp : Func_Op<"func", [
     bool isDeclaration() { return isExternal(); }
   }];
   let hasCustomAssemblyFormat = 1;
-  let hasVerifier = 1;
+  let hasRegionVerifier = 1;
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/Func/IR/FuncOps.cpp b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
index 2749ad5262f2b..d803e99154499 100644
--- a/mlir/lib/Dialect/Func/IR/FuncOps.cpp
+++ b/mlir/lib/Dialect/Func/IR/FuncOps.cpp
@@ -284,7 +284,7 @@ FuncOp FuncOp::clone() {
 // ReturnOp
 //===----------------------------------------------------------------------===//
 
-LogicalResult FuncOp::verify() {
+LogicalResult FuncOp::verifyRegions() {
   // External declarations have no body to check.
   if (isDeclaration())
     return success();
diff --git a/mlir/test/Dialect/Func/func-verifier-order-crash.mlir b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
new file mode 100644
index 0000000000000..7896f7d37c63f
--- /dev/null
+++ b/mlir/test/Dialect/Func/func-verifier-order-crash.mlir
@@ -0,0 +1,12 @@
+// After moving the return-type checks from FuncOp::verify() to
+// FuncOp::verifyRegions() (hasRegionVerifier=1), the terminator is verified
+// first and emits a clean diagnostic instead of crashing.
+//
+// RUN: mlir-opt %s -split-input-file -verify-diagnostics
+
+// -----
+
+func.func @func_verifier_runs_before_region() -> () {
+  // expected-error @below {{requires 'valid' unit attribute}}
+  test.crashing_return
+}
diff --git a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
index c243bd79a44a8..7cf728f933395 100644
--- a/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
+++ b/mlir/test/lib/Dialect/Test/TestOpDefs.cpp
@@ -1287,6 +1287,24 @@ LoopBlockTerminatorOp::getMutableSuccessorOperands(RegionSuccessor successor) {
   return getNextIterArgMutable();
 }
 
+//===----------------------------------------------------------------------===//
+// TestCrashingReturnOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult TestCrashingReturnOp::verify() {
+  if (!getValid())
+    return emitOpError("requires 'valid' unit attribute");
+  return success();
+}
+
+MutableOperandRange
+TestCrashingReturnOp::getMutableSuccessorOperands(RegionSuccessor successor) {
+  if (!getValid())
+    llvm::report_fatal_error("getMutableSuccessorOperands called on unverified "
+                             "test.crashing_return");
+  return getArgsMutable();
+}
+
 //===----------------------------------------------------------------------===//
 // SwitchWithNoBreakOp
 //===----------------------------------------------------------------------===//
diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td
index fe02536a1df5b..bd0b6e25efa53 100644
--- a/mlir/test/lib/Dialect/Test/TestOps.td
+++ b/mlir/test/lib/Dialect/Test/TestOps.td
@@ -2239,6 +2239,17 @@ def TestReturnOp : TEST_Op<"return", [Pure, ReturnLike, Terminator]> {
     [{ build($_builder, $_state, {}); }]>
   ];
 }
+// A return-like terminator used to test FuncOp verifier ordering.
+// When 'valid' unit-attr is absent the op is malformed; its verifier
+// emits an error, but getMutableSuccessorOperands() calls report_fatal_error
+// to expose the fact that FuncOp::verify() runs before the region is checked.
+def TestCrashingReturnOp : TEST_Op<"crashing_return", [
+    DeclareOpInterfaceMethods<RegionBranchTerminatorOpInterface>,
+    Terminator]> {
+  let arguments = (ins Variadic<AnyType>:$args, UnitAttr:$valid);
+  let assemblyFormat = "($args^ `:` type($args))? attr-dict";
+  let hasVerifier = 1;
+}
 def TestCastOp : TEST_Op<"cast">,
   Arguments<(ins Variadic<AnyType>)>, Results<(outs AnyType)>;
 def TestInvalidOp : TEST_Op<"invalid", [Terminator]>,