[Mlir-commits] [mlir] [mlir][Transform] Fix crash with invalid ir for transform libraries (PR #75649)

[Kunwar Grover]

https://github.com/Groverkss updated https://github.com/llvm/llvm-project/pull/75649

>From a871189a913e2867e516b6bd463c4b46d81f62c5 Mon Sep 17 00:00:00 2001
From: Kunwar Grover <groverkss at gmail.com>
Date: Sat, 16 Dec 2023 01:46:44 +0530
Subject: [PATCH 1/3] [mlir][Transform] Fix crash with invalid ir for transform
 libraries

---
 .../Transform/Transforms/TransformInterpreterUtils.cpp    | 6 ++++++
 .../Transform/include/test-interpreter-invalid.mlir       | 8 ++++++++
 2 files changed, 14 insertions(+)
 create mode 100644 mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir

diff --git a/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp b/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
index 3fa26bce150992..2f74b76f07b77b 100644
--- a/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
+++ b/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
@@ -109,6 +109,12 @@ LogicalResult transform::detail::parseTransformModuleFromFile(
   sourceMgr.AddNewSourceBuffer(std::move(memoryBuffer), llvm::SMLoc());
   transformModule =
       OwningOpRef<ModuleOp>(parseSourceFile<ModuleOp>(sourceMgr, context));
+  if (!transformModule) {
+    // Failed to parse the transform module.
+    // Don't need to emit an error here as the parsing should have already done
+    // that.
+    return failure();
+  }
   return mlir::verify(*transformModule);
 }
 
diff --git a/mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir
new file mode 100644
index 00000000000000..8b5641293d27e6
--- /dev/null
+++ b/mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir
@@ -0,0 +1,8 @@
+// RUN: mlir-opt %s --verify-diagnostics
+
+module attributes {transform.with_named_sequence} {
+  transform.named_sequence private @private_helper(%arg0: !transform.any_op {transform.readonly}) {
+    // expected-error @below {{expected ','}}
+    transform.test_print_remark_at_operand %arg0 "message" : !transform.any_op
+  }
+}

>From 182a743fae819427ad0352620250618c79ab8b3c Mon Sep 17 00:00:00 2001
From: Kunwar Grover <groverkss at gmail.com>
Date: Sat, 16 Dec 2023 18:19:13 +0530
Subject: [PATCH 2/3] Add better tests

---
 .../definitions-invalid.mlir}                              | 3 +++
 mlir/test/Dialect/Transform/preload-library-invalid.mlir   | 7 +++++++
 2 files changed, 10 insertions(+)
 rename mlir/test/Dialect/Transform/include/{test-interpreter-invalid.mlir => test-interpreter-library-invalid/definitions-invalid.mlir} (74%)
 create mode 100644 mlir/test/Dialect/Transform/preload-library-invalid.mlir

diff --git a/mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
similarity index 74%
rename from mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
index 8b5641293d27e6..ee6d1d909d9f36 100644
--- a/mlir/test/Dialect/Transform/include/test-interpreter-invalid.mlir
+++ b/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
@@ -1,5 +1,8 @@
 // RUN: mlir-opt %s --verify-diagnostics
 
+// The only thing we check here is that it should fail to parse. The other
+// check is in the preload test.
+
 module attributes {transform.with_named_sequence} {
   transform.named_sequence private @private_helper(%arg0: !transform.any_op {transform.readonly}) {
     // expected-error @below {{expected ','}}
diff --git a/mlir/test/Dialect/Transform/preload-library-invalid.mlir b/mlir/test/Dialect/Transform/preload-library-invalid.mlir
new file mode 100644
index 00000000000000..9abb849e4d27eb
--- /dev/null
+++ b/mlir/test/Dialect/Transform/preload-library-invalid.mlir
@@ -0,0 +1,7 @@
+// RUN: mlir-opt %s \
+// RUN:   -transform-preload-library=transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library-invalid \
+// RUN:   -transform-interpreter=entry-point=private_helper \
+// RUN:   -verify-diagnostics
+
+// This test checks if the preload mechanism fails gracefully when passed an
+// invalid transform file.

>From b657ea4f26269d7cc3e0fcd42bdecef5df82a836 Mon Sep 17 00:00:00 2001
From: Kunwar Grover <groverkss at gmail.com>
Date: Tue, 19 Dec 2023 22:33:11 +0530
Subject: [PATCH 3/3] Address comment

---
 .../test-interpreter-library-invalid/definitions-invalid.mlir   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
index ee6d1d909d9f36..08e53a8d553c36 100644
--- a/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
+++ b/mlir/test/Dialect/Transform/include/test-interpreter-library-invalid/definitions-invalid.mlir
@@ -6,6 +6,6 @@
 module attributes {transform.with_named_sequence} {
   transform.named_sequence private @private_helper(%arg0: !transform.any_op {transform.readonly}) {
     // expected-error @below {{expected ','}}
-    transform.test_print_remark_at_operand %arg0 "message" : !transform.any_op
+    transform.test_print_remark_at_operand %arg0 "should have ',' prior to this" : !transform.any_op
   }
 }