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

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

<details>
<summary>Changes</summary>

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.

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


5 Files Affected:

- (modified) mlir/include/mlir/Dialect/Func/IR/FuncOps.td (+1-1) 
- (modified) mlir/lib/Dialect/Func/IR/FuncOps.cpp (+1-1) 
- (added) mlir/test/Dialect/Func/func-verifier-order-crash.mlir (+12) 
- (modified) mlir/test/lib/Dialect/Test/TestOpDefs.cpp (+18) 
- (modified) mlir/test/lib/Dialect/Test/TestOps.td (+11) 


``````````diff
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]>,

``````````

</details>


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