[Mlir-commits] [mlir] ab2e224 - Fix UB passing nullptr to an EmptyProperties class when building OpAdaptor

[Mehdi Amini]

Author: Mehdi Amini
Date: 2023-05-05T16:50:52-07:00
New Revision: ab2e224e1935350d78de4bfb6cb3f2e8628df23d

URL: https://github.com/llvm/llvm-project/commit/ab2e224e1935350d78de4bfb6cb3f2e8628df23d
DIFF: https://github.com/llvm/llvm-project/commit/ab2e224e1935350d78de4bfb6cb3f2e8628df23d.diff

LOG: Fix UB passing nullptr to an EmptyProperties class when building OpAdaptor

A new forwarding constructor is introduced on the adaptor to take directly
an OpaqueProperties object and do the nullptr checking and casting to avoid
the boilerplate at callsites.

Differential Revision: https://reviews.llvm.org/D150003

Added: 
    

Modified: 
    mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
    mlir/test/mlir-tblgen/op-decl-and-defs.td
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 6b0d2b5d014d5..4dfc09f11b911 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -1357,8 +1357,8 @@ LogicalResult ExtractStridedMetadataOp::inferReturnTypes(
     MLIRContext *context, std::optional<Location> location, ValueRange operands,
     DictionaryAttr attributes, OpaqueProperties properties, RegionRange regions,
     SmallVectorImpl<Type> &inferredReturnTypes) {
-  ExtractStridedMetadataOpAdaptor extractAdaptor(
-      operands, attributes, *properties.as<EmptyProperties *>(), regions);
+  ExtractStridedMetadataOpAdaptor extractAdaptor(operands, attributes,
+                                                 properties);
   auto sourceType = extractAdaptor.getSource().getType().dyn_cast<MemRefType>();
   if (!sourceType)
     return failure();

diff  --git a/mlir/test/mlir-tblgen/op-decl-and-defs.td b/mlir/test/mlir-tblgen/op-decl-and-defs.td
index 618197c76c2a0..aad7ea4437e78 100644
--- a/mlir/test/mlir-tblgen/op-decl-and-defs.td
+++ b/mlir/test/mlir-tblgen/op-decl-and-defs.td
@@ -126,7 +126,7 @@ def NS_AOp : NS_Op<"a_op", [IsolatedFromAbove, IsolatedFromAbove]> {
 
 // DEFS-LABEL: NS::AOp definitions
 
-// DEFS: AOpGenericAdaptorBase::AOpGenericAdaptorBase(::mlir::DictionaryAttr attrs, ::mlir::EmptyProperties properties, ::mlir::RegionRange regions) : odsAttrs(attrs), odsRegions(regions)
+// DEFS: AOpGenericAdaptorBase::AOpGenericAdaptorBase(::mlir::DictionaryAttr attrs, const ::mlir::EmptyProperties &properties, ::mlir::RegionRange regions) : odsAttrs(attrs), odsRegions(regions)
 // DEFS: ::mlir::RegionRange AOpGenericAdaptorBase::getSomeRegions()
 // DEFS-NEXT: return odsRegions.drop_front(1);
 // DEFS: ::mlir::RegionRange AOpGenericAdaptorBase::getRegions()

diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index dc257fd97b0e0..b090de958faff 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3539,7 +3539,8 @@ OpOperandAdaptorEmitter::OpOperandAdaptorEmitter(
     if (useProperties)
       paramList.emplace_back("const Properties &", "properties", "{}");
     else
-      paramList.emplace_back("::mlir::EmptyProperties", "properties", "{}");
+      paramList.emplace_back("const ::mlir::EmptyProperties &", "properties",
+                             "{}");
     paramList.emplace_back("::mlir::RegionRange", "regions", "{}");
     auto *baseConstructor = genericAdaptorBase.addConstructor(paramList);
     baseConstructor->addMemberInitializer("odsAttrs", "attrs");
@@ -3554,9 +3555,33 @@ OpOperandAdaptorEmitter::OpOperandAdaptorEmitter(
         op.getOperationName());
 
     paramList.insert(paramList.begin(), MethodParameter("RangeT", "values"));
-    auto *constructor = genericAdaptor.addConstructor(std::move(paramList));
+    auto *constructor = genericAdaptor.addConstructor(paramList);
     constructor->addMemberInitializer("Base", "attrs, properties, regions");
     constructor->addMemberInitializer("odsOperands", "values");
+
+    // Add a forwarding constructor to the previous one that accepts
+    // OpaqueProperties instead and check for null and perform the cast to the
+    // actual properties type.
+    paramList[1] = MethodParameter("::mlir::DictionaryAttr", "attrs");
+    paramList[2] = MethodParameter("::mlir::OpaqueProperties", "properties");
+    auto *opaquePropertiesConstructor =
+        genericAdaptor.addConstructor(std::move(paramList));
+    if (useProperties) {
+      opaquePropertiesConstructor->addMemberInitializer(
+          genericAdaptor.getClassName(),
+          "values, "
+          "attrs, "
+          "(properties ? *properties.as<Properties *>() : Properties{}), "
+          "regions");
+    } else {
+      opaquePropertiesConstructor->addMemberInitializer(
+          genericAdaptor.getClassName(),
+          "values, "
+          "attrs, "
+          "(properties ? *properties.as<::mlir::EmptyProperties *>() : "
+          "::mlir::EmptyProperties{}), "
+          "regions");
+    }
   }
 
   std::string sizeAttrInit;