Free memory leak on duplicate interface registration

[mehdi_amini]

Summary

I guess this is why we should use unique_ptr as much as possible.
Also fix the InterfaceAttachmentTest.cpp test.

mlir/lib/Support/InterfaceSupport.cpp

void detail::InterfaceMap::insert(
   // Insert directly into the right position to keep the interfaces sorted.
   for (auto &element : elements) {
     TypeID id = element.first;
     auto *it = llvm::lower_bound(interfaces, id, [](const auto &it, TypeID id) {
       return compare(it.first, id);
     });
     if (it != interfaces.end() && it->first == id) {
       LLVM_DEBUG(llvm::dbgs() << "Ignoring repeated interface registration");
+      free(element.second);
       continue;
     }
     interfaces.insert(it, element);
   }
 }

mlir/unittests/IR/InterfaceAttachmentTest.cpp

 #include "mlir/IR/BuiltinDialect.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "gtest/gtest.h"
 
 #include "../../test/lib/Dialect/Test/TestAttributes.h"
 #include "../../test/lib/Dialect/Test/TestDialect.h"
 #include "../../test/lib/Dialect/Test/TestTypes.h"
+#include "mlir/IR/OwningOpRef.h"
 
 using namespace mlir;
 using namespace test;
 
 namespace {
 
 /// External interface model for the integer type. Only provides non-default
 /// methods.
struct TestExternalOpOverridingModel
 
   static unsigned getNameLengthMinusArg(unsigned arg) { return 21; }
 };
 
 TEST(InterfaceAttachment, Operation) {
   MLIRContext context;
 
   // Initially, the operation doesn't have the interface.
-  auto moduleOp = ModuleOp::create(UnknownLoc::get(&context));
-  ASSERT_FALSE(isa<TestExternalOpInterface>(moduleOp.getOperation()));
+  OwningOpRef<ModuleOp> moduleOp = ModuleOp::create(UnknownLoc::get(&context));
+  ASSERT_FALSE(isa<TestExternalOpInterface>(moduleOp->getOperation()));
 
   // We can attach an external interface and now the operaiton has it.
   ModuleOp::attachInterface<TestExternalOpModel>(context);
-  auto iface = dyn_cast<TestExternalOpInterface>(moduleOp.getOperation());
+  auto iface = dyn_cast<TestExternalOpInterface>(moduleOp->getOperation());
   ASSERT_TRUE(iface != nullptr);
   EXPECT_EQ(iface.getNameLengthPlusArg(10), 24u);
   EXPECT_EQ(iface.getNameLengthTimesArg(3), 42u);
   EXPECT_EQ(iface.getNameLengthPlusArgTwice(18), 50u);
   EXPECT_EQ(iface.getNameLengthMinusArg(5), 9u);
 
   // Default implementation can be overridden.
-  auto funcOp = FuncOp::create(UnknownLoc::get(&context), "function",
-                               FunctionType::get(&context, {}, {}));
-  ASSERT_FALSE(isa<TestExternalOpInterface>(funcOp.getOperation()));
+  OwningOpRef<FuncOp> funcOp =
+      FuncOp::create(UnknownLoc::get(&context), "function",
+  ASSERT_FALSE(isa<TestExternalOpInterface>(funcOp->getOperation()));
   FuncOp::attachInterface<TestExternalOpOverridingModel>(context);
-  iface = dyn_cast<TestExternalOpInterface>(funcOp.getOperation());
+  iface = dyn_cast<TestExternalOpInterface>(funcOp->getOperation());
   ASSERT_TRUE(iface != nullptr);
   EXPECT_EQ(iface.getNameLengthPlusArg(10), 22u);
   EXPECT_EQ(iface.getNameLengthTimesArg(0), 42u);
   EXPECT_EQ(iface.getNameLengthPlusArgTwice(8), 28u);
   EXPECT_EQ(iface.getNameLengthMinusArg(1000), 21u);
 
   // Another context doesn't have the interfaces registered.
   MLIRContext other;
-  auto otherModuleOp = ModuleOp::create(UnknownLoc::get(&other));
-  ASSERT_FALSE(isa<TestExternalOpInterface>(otherModuleOp.getOperation()));
+  OwningOpRef<ModuleOp> otherModuleOp =
+      ModuleOp::create(UnknownLoc::get(&other));
+  ASSERT_FALSE(isa<TestExternalOpInterface>(otherModuleOp->getOperation()));
 }
 
 template <class ConcreteOp>
 struct TestExternalTestOpModel
     : public TestExternalOpInterface::ExternalModel<
           TestExternalTestOpModel<ConcreteOp>, ConcreteOp> {
   unsigned getNameLengthPlusArg(Operation *op, unsigned arg) const {
     return op->getName().getStringRef().size() + arg;
TEST(InterfaceAttachment, OperationDelayedContextConstruct) {
   registry.addOpInterface<test::OpJ, TestExternalTestOpModel<test::OpJ>>();
   registry.addOpInterface<test::OpH, TestExternalTestOpModel<test::OpH>>();
 
   // Construct the context directly from a registry. The interfaces are expected
   // to be readily available on operations.
   MLIRContext context(registry);
   context.loadDialect<test::TestDialect>();
 
-  ModuleOp module = ModuleOp::create(UnknownLoc::get(&context));
-  OpBuilder builder(module);
+  OwningOpRef<ModuleOp> module = ModuleOp::create(UnknownLoc::get(&context));
+  OpBuilder builder(module->getBody(), module->getBody()->begin());
   auto opJ =
       builder.create<test::OpJ>(builder.getUnknownLoc(), builder.getI32Type());
   auto opH =
       builder.create<test::OpH>(builder.getUnknownLoc(), opJ.getResult());
   auto opI =
       builder.create<test::OpI>(builder.getUnknownLoc(), opJ.getResult());
 
-  EXPECT_TRUE(isa<TestExternalOpInterface>(module.getOperation()));
+  EXPECT_TRUE(isa<TestExternalOpInterface>(module->getOperation()));
   EXPECT_TRUE(isa<TestExternalOpInterface>(opJ.getOperation()));
   EXPECT_TRUE(isa<TestExternalOpInterface>(opH.getOperation()));
   EXPECT_FALSE(isa<TestExternalOpInterface>(opI.getOperation()));
 }
 
 TEST(InterfaceAttachment, OperationDelayedContextAppend) {
   DialectRegistry registry;
   registry.insert<test::TestDialect>();
   registry.addOpInterface<ModuleOp, TestExternalOpModel>();
   registry.addOpInterface<test::OpJ, TestExternalTestOpModel<test::OpJ>>();
   registry.addOpInterface<test::OpH, TestExternalTestOpModel<test::OpH>>();
 
   // Construct the context, create ops, and only then append the registry. The
   // interfaces are expected to be available after appending the registry.
   MLIRContext context;
   context.loadDialect<test::TestDialect>();
 
-  ModuleOp module = ModuleOp::create(UnknownLoc::get(&context));
-  OpBuilder builder(module);
+  OwningOpRef<ModuleOp> module = ModuleOp::create(UnknownLoc::get(&context));
+  OpBuilder builder(module->getBody(), module->getBody()->begin());
   auto opJ =
       builder.create<test::OpJ>(builder.getUnknownLoc(), builder.getI32Type());
   auto opH =
       builder.create<test::OpH>(builder.getUnknownLoc(), opJ.getResult());
   auto opI =
       builder.create<test::OpI>(builder.getUnknownLoc(), opJ.getResult());
 
-  EXPECT_FALSE(isa<TestExternalOpInterface>(module.getOperation()));
+  EXPECT_FALSE(isa<TestExternalOpInterface>(module->getOperation()));
   EXPECT_FALSE(isa<TestExternalOpInterface>(opJ.getOperation()));
   EXPECT_FALSE(isa<TestExternalOpInterface>(opH.getOperation()));
   EXPECT_FALSE(isa<TestExternalOpInterface>(opI.getOperation()));
 
   context.appendDialectRegistry(registry);
 
-  EXPECT_TRUE(isa<TestExternalOpInterface>(module.getOperation()));
+  EXPECT_TRUE(isa<TestExternalOpInterface>(module->getOperation()));
   EXPECT_TRUE(isa<TestExternalOpInterface>(opJ.getOperation()));
   EXPECT_TRUE(isa<TestExternalOpInterface>(opH.getOperation()));
   EXPECT_FALSE(isa<TestExternalOpInterface>(opI.getOperation()));
 }
 
 } // end namespace