[Mlir-commits] [mlir] [mlir][Parser] Fix crash when resolving invalid operands with missing location (PR #128163)

[Matthias Springer]

https://github.com/matthias-springer created https://github.com/llvm/llvm-project/pull/128163

When `resolveOperands` reports an error and no valid `SMLoc` was provided, fall back to the beginning of the op instead of crashing.

E.g., this is currently the case when parsing the following op with a type but without any operands:
```
let assemblyFormat = "$str (`,` $args^)? attr-dict (`:` type($args)^)?";
```

In the ODS-generated C++, the `SMLoc` is populated when parsing the optional group containing `$args`. However, this group is missing in the test case.

There are likely additional hand-written parsers that suffer from the same problem.

Note: I tried emitting a second `SMLoc` for the optional type group in the `OpFormatGen.cpp`, but this adds quite a bit of complexity in the code base for little improvement in user experience.


>From a9e45a572e08b781de359bca81838cd9012de0cd Mon Sep 17 00:00:00 2001
From: Matthias Springer <mspringer at nvidia.com>
Date: Fri, 21 Feb 2025 11:57:26 +0100
Subject: [PATCH] [mlir][Parser] Fix crash when resolving invalid operands with
 missing location

---
 mlir/include/mlir/IR/OpImplementation.h     | 6 ++++--
 mlir/test/IR/invalid-ops.mlir               | 5 +++++
 mlir/test/lib/Dialect/Test/TestOpsSyntax.td | 5 +++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index a863e881ee7c8..d80bff2d78217 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -1603,10 +1603,12 @@ class OpAsmParser : public AsmParser {
                   SmallVectorImpl<Value> &result) {
     size_t operandSize = llvm::range_size(operands);
     size_t typeSize = llvm::range_size(types);
-    if (operandSize != typeSize)
-      return emitError(loc)
+    if (operandSize != typeSize) {
+      // If no location was provided, report errors at the beginning of the op.
+      return emitError(loc.isValid() ? loc : getNameLoc())
              << "number of operands and types do not match: got " << operandSize
              << " operands and " << typeSize << " types";
+    }
 
     for (auto [operand, type] : llvm::zip_equal(operands, types))
       if (resolveOperand(operand, type, result))
diff --git a/mlir/test/IR/invalid-ops.mlir b/mlir/test/IR/invalid-ops.mlir
index 6672c8840ffde..c7cef97c246e8 100644
--- a/mlir/test/IR/invalid-ops.mlir
+++ b/mlir/test/IR/invalid-ops.mlir
@@ -118,3 +118,8 @@ func.func @invalid_splat(%v : f32) { // expected-note {{prior use here}}
 
 // expected-error at +1 {{expected ':' after block name}}
 "g"()({^a:^b })
+
+// -----
+
+// expected-error at +1 {{number of operands and types do not match: got 0 operands and 1 types}}
+test.variadic_args_types_split "hello_world" : i32
diff --git a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
index 2848cb994231b..9c199f0c3b6fc 100644
--- a/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
+++ b/mlir/test/lib/Dialect/Test/TestOpsSyntax.td
@@ -767,4 +767,9 @@ def FormatInferTypeVariadicOperandsOp
   }];
 }
 
+def VariadicArgsTypesSplit : TEST_Op<"variadic_args_types_split"> {
+  let arguments = (ins StrAttr:$str, Variadic<AnyType>:$args);
+  let assemblyFormat = "$str (`,` $args^)? attr-dict (`:` type($args)^)?";
+}
+
 #endif  // TEST_OPS_SYNTAX