Avoid memory leaks when passing arguments to drivers (#283)

Use a new ArgumentList class instead of malloced pointers to pass
arguments to drivers.

This new class will not own all arguments passed, just those which
were created in place (via emplace_back) to avoid unnecessary
copying.

Signed-off-by: Victor Perez <[email protected]>

Signed-off-by: Victor Perez <[email protected]>

Author: victor-eds

tools/cgeist/ArgumentList.h

@@ -0,0 +1,50 @@
+//===- ArgumentList.h -------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef MLIR_TOOLS_CGEIST_ARGUMENTLIST_H
+#define MLIR_TOOLS_CGEIST_ARGUMENTLIST_H
+
+#include <llvm/ADT/ArrayRef.h>
+#include <llvm/Support/raw_ostream.h>
+
+namespace mlirclang {
+/// Class to pass options to a compilation tool.
+class ArgumentList {
+public:
+  /// Add argument.
+  ///
+  /// The element stored will not be owned by this.
+  void push_back(llvm::StringRef Arg) { Args.push_back(Arg.data()); }
+
+  /// Add argument and ensure it will be valid before this passer's destruction.
+  ///
+  /// The element stored will be owned by this.
+  template <typename... ArgTy> void emplace_back(ArgTy &&...Args) {
+    // Store as a string
+    std::string Buffer;
+    llvm::raw_string_ostream Stream(Buffer);
+    (Stream << ... << Args);
+    Storage.push_back(Stream.str());
+    push_back(Storage.back());
+  }
+
+  /// Return the underling argument list.
+  ///
+  /// The return value of this operation could be invalidated by subsequent
+  /// calls to push_back() or emplace_back().
+  llvm::ArrayRef<const char *> getArguments() const { return Args; }
+
+private:
+  /// Helper storage.
+  llvm::SmallVector<std::string> Storage;
+  /// List of arguments
+  llvm::SmallVector<const char *> Args;
+};
+} // end namespace mlirclang
+
+#endif

tools/cgeist/Lib/clang-mlir.cc

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang-mlir.h"
+#include "../ArgumentList.h"
 #include "TypeUtils.h"
 #include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/DLTI/DLTI.h"
@@ -5510,50 +5511,30 @@ static bool parseMLIR(const char *Argv0, std::vector<std::string> filenames,
   const char *binary = Argv0; // CudaLower ? "clang++" : "clang";
   const unique_ptr<Driver> driver(
       new Driver(binary, llvm::sys::getDefaultTargetTriple(), Diags));
-  std::vector<const char *> Argv;
+  mlirclang::ArgumentList Argv;
   Argv.push_back(binary);
-  for (auto a : filenames) {
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+  for (const auto &filename : filenames) {
+    Argv.push_back(filename);
   }
   if (FOpenMP)
     Argv.push_back("-fopenmp");
   if (TargetTripleOpt != "") {
-    char *chars = (char *)malloc(TargetTripleOpt.length() + 1);
-    memcpy(chars, TargetTripleOpt.data(), TargetTripleOpt.length());
-    chars[TargetTripleOpt.length()] = 0;
     Argv.push_back("-target");
-    Argv.push_back(chars);
+    Argv.push_back(TargetTripleOpt);
   }
   if (McpuOpt != "") {
-    auto a = "-mcpu=" + McpuOpt;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("-mcpu=", McpuOpt);
   }
   if (Standard != "") {
-    auto a = "-std=" + Standard;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("-std=", Standard);
   }
   if (ResourceDir != "") {
     Argv.push_back("-resource-dir");
-    char *chars = (char *)malloc(ResourceDir.length() + 1);
-    memcpy(chars, ResourceDir.data(), ResourceDir.length());
-    chars[ResourceDir.length()] = 0;
-    Argv.push_back(chars);
+    Argv.push_back(ResourceDir);
   }
   if (SysRoot != "") {
     Argv.push_back("--sysroot");
-    char *chars = (char *)malloc(SysRoot.length() + 1);
-    memcpy(chars, SysRoot.data(), SysRoot.length());
-    chars[SysRoot.length()] = 0;
-    Argv.push_back(chars);
+    Argv.push_back(SysRoot);
   }
   if (Verbose) {
     Argv.push_back("-v");
@@ -5565,53 +5546,30 @@ static bool parseMLIR(const char *Argv0, std::vector<std::string> filenames,
     Argv.push_back("-nocudalib");
   }
   if (CUDAGPUArch != "") {
-    auto a = "--cuda-gpu-arch=" + CUDAGPUArch;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("--cuda-gpu-arch=", CUDAGPUArch);
   }
   if (CUDAPath != "") {
-    auto a = "--cuda-path=" + CUDAPath;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("--cuda-path=", CUDAPath);
   }
   if (MArch != "") {
-    auto a = "-march=" + MArch;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("-march=", MArch);
   }
-  for (auto a : includeDirs) {
+  for (const auto &dir : includeDirs) {
     Argv.push_back("-I");
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
-  }
-  for (auto a : defines) {
-    char *chars = (char *)malloc(a.length() + 3);
-    chars[0] = '-';
-    chars[1] = 'D';
-    memcpy(chars + 2, a.data(), a.length());
-    chars[2 + a.length()] = 0;
-    Argv.push_back(chars);
-  }
-  for (auto a : Includes) {
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
+    Argv.push_back(dir);
+  }
+  for (const auto &define : defines) {
+    Argv.emplace_back("-D", define);
+  }
+  for (const auto &Include : Includes) {
     Argv.push_back("-include");
-    Argv.push_back(chars);
+    Argv.push_back(Include);
   }
 
   Argv.push_back("-emit-ast");
 
   const unique_ptr<Compilation> compilation(
-      driver->BuildCompilation(llvm::ArrayRef<const char *>(Argv)));
+      driver->BuildCompilation(Argv.getArguments()));
   JobList &Jobs = compilation->getJobs();
   if (Jobs.size() < 1)
     return false;

tools/cgeist/driver.cc

@@ -54,6 +54,8 @@
 #include "polygeist/Dialect.h"
 #include "polygeist/Passes/Passes.h"
 
+#include "ArgumentList.h"
+
 using namespace llvm;
 
 static cl::OptionCategory toolOptions("clang to mlir - tool options");
@@ -263,7 +265,7 @@ int emitBinary(char *Argv0, const char *filename,
   const char *binary = Argv0;
   const unique_ptr<Driver> driver(new Driver(binary, TargetTriple, Diags));
   driver->CC1Main = &ExecuteCC1Tool;
-  std::vector<const char *> Argv;
+  mlirclang::ArgumentList Argv;
   Argv.push_back(Argv0);
   // Argv.push_back("-x");
   // Argv.push_back("ir");
@@ -272,27 +274,16 @@ int emitBinary(char *Argv0, const char *filename,
     Argv.push_back("-fopenmp");
   if (ResourceDir != "") {
     Argv.push_back("-resource-dir");
-    char *chars = (char *)malloc(ResourceDir.length() + 1);
-    memcpy(chars, ResourceDir.data(), ResourceDir.length());
-    chars[ResourceDir.length()] = 0;
-    Argv.push_back(chars);
+    Argv.push_back(ResourceDir);
   }
   if (Verbose) {
     Argv.push_back("-v");
   }
   if (CUDAGPUArch != "") {
-    auto a = "--cuda-gpu-arch=" + CUDAGPUArch;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("--cuda-gpu-arch=", CUDAGPUArch);
   }
   if (CUDAPath != "") {
-    auto a = "--cuda-path=" + CUDAPath;
-    char *chars = (char *)malloc(a.length() + 1);
-    memcpy(chars, a.data(), a.length());
-    chars[a.length()] = 0;
-    Argv.push_back(chars);
+    Argv.emplace_back("--cuda-path=", CUDAPath);
   }
   if (Opt0) {
     Argv.push_back("-O0");
@@ -308,16 +299,13 @@ int emitBinary(char *Argv0, const char *filename,
   }
   if (Output != "") {
     Argv.push_back("-o");
-    char *chars = (char *)malloc(Output.length() + 1);
-    memcpy(chars, Output.data(), Output.length());
-    chars[Output.length()] = 0;
-    Argv.push_back(chars);
+    Argv.push_back(Output);
   }
   for (const auto *arg : LinkArgs)
     Argv.push_back(arg);
 
   const unique_ptr<Compilation> compilation(
-      driver->BuildCompilation(llvm::ArrayRef<const char *>(Argv)));
+      driver->BuildCompilation(Argv.getArguments()));
 
   if (ResourceDir != "")
     driver->ResourceDir = ResourceDir;