Refactor ExecuteAndWait to take StringRefs.

This simplifies some code which had StringRefs to begin with, and
makes other code more complicated which had const char* to begin
with.

In the end, I think this makes for a more idiomatic and platform
agnostic API.  Not all platforms launch process with null terminated
c-string arrays for the environment pointer and argv, but the api
was designed that way because it allowed easy pass-through for
posix-based platforms.  There's a little additional overhead now
since on posix based platforms we'll be takign StringRefs which
were constructed from null terminated strings and then copying
them to null terminate them again, but from a readability and
usability standpoint of the API user, I think this API signature
is strictly better.

llvm-svn: 334518

5 files changed, 80 insertions, 73 deletions

diff --git a/llvm/lib/Support/GraphWriter.cpp b/llvm/lib/Support/GraphWriter.cpp
index 794c6ba3a85..9335daffc3e 100644
--- a/llvm/lib/Support/GraphWriter.cpp
+++ b/llvm/lib/Support/GraphWriter.cpp
@@ -91,20 +91,18 @@ std::string llvm::createGraphFilename(const Twine &Name, int &FD) {
 }
 
 // Execute the graph viewer. Return true if there were errors.
-static bool ExecGraphViewer(StringRef ExecPath, std::vector<const char *> &args,
+static bool ExecGraphViewer(StringRef ExecPath, std::vector<StringRef> &args,
                             StringRef Filename, bool wait,
                             std::string &ErrMsg) {
-  assert(args.back() == nullptr);
   if (wait) {
-    if (sys::ExecuteAndWait(ExecPath, args.data(), nullptr, {}, 0, 0,
-                            &ErrMsg)) {
+    if (sys::ExecuteAndWait(ExecPath, args, None, {}, 0, 0, &ErrMsg)) {
       errs() << "Error: " << ErrMsg << "\n";
       return true;
     }
     sys::fs::remove(Filename);
     errs() << " done. \n";
   } else {
-    sys::ExecuteNoWait(ExecPath, args.data(), nullptr, {}, 0, &ErrMsg);
+    sys::ExecuteNoWait(ExecPath, args, None, {}, 0, &ErrMsg);
     errs() << "Remember to erase graph file: " << Filename << "\n";
   }
   return false;
@@ -158,22 +156,20 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
 #ifdef __APPLE__
   wait &= !ViewBackground;
   if (S.TryFindProgram("open", ViewerPath)) {
-    std::vector<const char *> args;
-    args.push_back(ViewerPath.c_str());
+    std::vector<StringRef> args;
+    args.push_back(ViewerPath);
     if (wait)
       args.push_back("-W");
-    args.push_back(Filename.c_str());
-    args.push_back(nullptr);
+    args.push_back(Filename);
     errs() << "Trying 'open' program... ";
     if (!ExecGraphViewer(ViewerPath, args, Filename, wait, ErrMsg))
       return false;
   }
 #endif
   if (S.TryFindProgram("xdg-open", ViewerPath)) {
-    std::vector<const char *> args;
-    args.push_back(ViewerPath.c_str());
-    args.push_back(Filename.c_str());
-    args.push_back(nullptr);
+    std::vector<StringRef> args;
+    args.push_back(ViewerPath);
+    args.push_back(Filename);
     errs() << "Trying 'xdg-open' program... ";
     if (!ExecGraphViewer(ViewerPath, args, Filename, wait, ErrMsg))
       return false;
@@ -181,10 +177,9 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
 
   // Graphviz
   if (S.TryFindProgram("Graphviz", ViewerPath)) {
-    std::vector<const char *> args;
-    args.push_back(ViewerPath.c_str());
-    args.push_back(Filename.c_str());
-    args.push_back(nullptr);
+    std::vector<StringRef> args;
+    args.push_back(ViewerPath);
+    args.push_back(Filename);
 
     errs() << "Running 'Graphviz' program... ";
     return ExecGraphViewer(ViewerPath, args, Filename, wait, ErrMsg);
@@ -192,15 +187,13 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
 
   // xdot
   if (S.TryFindProgram("xdot|xdot.py", ViewerPath)) {
-    std::vector<const char *> args;
-    args.push_back(ViewerPath.c_str());
-    args.push_back(Filename.c_str());
+    std::vector<StringRef> args;
+    args.push_back(ViewerPath);
+    args.push_back(Filename);
 
     args.push_back("-f");
     args.push_back(getProgramName(program));
 
-    args.push_back(nullptr);
-
     errs() << "Running 'xdot.py' program... ";
     return ExecGraphViewer(ViewerPath, args, Filename, wait, ErrMsg);
   }
@@ -235,18 +228,17 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
     std::string OutputFilename =
         Filename + (Viewer == VK_CmdStart ? ".pdf" : ".ps");
 
-    std::vector<const char *> args;
-    args.push_back(GeneratorPath.c_str());
+    std::vector<StringRef> args;
+    args.push_back(GeneratorPath);
     if (Viewer == VK_CmdStart)
       args.push_back("-Tpdf");
     else
       args.push_back("-Tps");
     args.push_back("-Nfontname=Courier");
     args.push_back("-Gsize=7.5,10");
-    args.push_back(Filename.c_str());
+    args.push_back(Filename);
     args.push_back("-o");
-    args.push_back(OutputFilename.c_str());
-    args.push_back(nullptr);
+    args.push_back(OutputFilename);
 
     errs() << "Running '" << GeneratorPath << "' program... ";
 
@@ -258,31 +250,30 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
     std::string StartArg;
 
     args.clear();
-    args.push_back(ViewerPath.c_str());
+    args.push_back(ViewerPath);
     switch (Viewer) {
     case VK_OSXOpen:
       args.push_back("-W");
-      args.push_back(OutputFilename.c_str());
+      args.push_back(OutputFilename);
       break;
     case VK_XDGOpen:
       wait = false;
-      args.push_back(OutputFilename.c_str());
+      args.push_back(OutputFilename);
       break;
     case VK_Ghostview:
       args.push_back("--spartan");
-      args.push_back(OutputFilename.c_str());
+      args.push_back(OutputFilename);
       break;
     case VK_CmdStart:
       args.push_back("/S");
       args.push_back("/C");
       StartArg =
           (StringRef("start ") + (wait ? "/WAIT " : "") + OutputFilename).str();
-      args.push_back(StartArg.c_str());
+      args.push_back(StartArg);
       break;
     case VK_None:
       llvm_unreachable("Invalid viewer");
     }
-    args.push_back(nullptr);
 
     ErrMsg.clear();
     return ExecGraphViewer(ViewerPath, args, OutputFilename, wait, ErrMsg);
@@ -290,10 +281,9 @@ bool llvm::DisplayGraph(StringRef FilenameRef, bool wait,
 
   // dotty
   if (S.TryFindProgram("dotty", ViewerPath)) {
-    std::vector<const char *> args;
-    args.push_back(ViewerPath.c_str());
-    args.push_back(Filename.c_str());
-    args.push_back(nullptr);
+    std::vector<StringRef> args;
+    args.push_back(ViewerPath);
+    args.push_back(Filename);
 
 // Dotty spawns another app and doesn't wait until it returns
 #ifdef _WIN32
diff --git a/llvm/lib/Support/Program.cpp b/llvm/lib/Support/Program.cpp
index 8c56fb6db2d..63cdcdaabee 100644
--- a/llvm/lib/Support/Program.cpp
+++ b/llvm/lib/Support/Program.cpp
@@ -23,17 +23,19 @@ using namespace sys;
 //===          independent code.
 //===----------------------------------------------------------------------===//
 
-static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Env, ArrayRef<Optional<StringRef>> Redirects,
+static bool Execute(ProcessInfo &PI, StringRef Program,
+                    ArrayRef<StringRef> Args, Optional<ArrayRef<StringRef>> Env,
+                    ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg);
 
-int sys::ExecuteAndWait(StringRef Program, const char **Args, const char **Envp,
+int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
+                        Optional<ArrayRef<StringRef>> Env,
                         ArrayRef<Optional<StringRef>> Redirects,
                         unsigned SecondsToWait, unsigned MemoryLimit,
                         std::string *ErrMsg, bool *ExecutionFailed) {
   assert(Redirects.empty() || Redirects.size() == 3);
   ProcessInfo PI;
-  if (Execute(PI, Program, Args, Envp, Redirects, MemoryLimit, ErrMsg)) {
+  if (Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg)) {
     if (ExecutionFailed)
       *ExecutionFailed = false;
     ProcessInfo Result = Wait(
@@ -47,8 +49,8 @@ int sys::ExecuteAndWait(StringRef Program, const char **Args, const char **Envp,
   return -1;
 }
 
-ProcessInfo sys::ExecuteNoWait(StringRef Program, const char **Args,
-                               const char **Envp,
+ProcessInfo sys::ExecuteNoWait(StringRef Program, ArrayRef<StringRef> Args,
+                               Optional<ArrayRef<StringRef>> Env,
                                ArrayRef<Optional<StringRef>> Redirects,
                                unsigned MemoryLimit, std::string *ErrMsg,
                                bool *ExecutionFailed) {
@@ -56,7 +58,7 @@ ProcessInfo sys::ExecuteNoWait(StringRef Program, const char **Args,
   ProcessInfo PI;
   if (ExecutionFailed)
     *ExecutionFailed = false;
-  if (!Execute(PI, Program, Args, Envp, Redirects, MemoryLimit, ErrMsg))
+  if (!Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg))
     if (ExecutionFailed)
       *ExecutionFailed = true;
 
diff --git a/llvm/lib/Support/Signals.cpp b/llvm/lib/Support/Signals.cpp
index 76a3c6bed41..6534ff69b84 100644
--- a/llvm/lib/Support/Signals.cpp
+++ b/llvm/lib/Support/Signals.cpp
@@ -154,17 +154,18 @@ static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace,
     }
   }
 
-  Optional<StringRef> Redirects[] = {InputFile.str(), OutputFile.str(), llvm::None};
-  const char *Args[] = {"llvm-symbolizer", "--functions=linkage", "--inlining",
+  Optional<StringRef> Redirects[] = {StringRef(InputFile),
+                                     StringRef(OutputFile), llvm::None};
+  StringRef Args[] = {"llvm-symbolizer", "--functions=linkage", "--inlining",
 #ifdef _WIN32
-                        // Pass --relative-address on Windows so that we don't
-                        // have to add ImageBase from PE file.
-                        // FIXME: Make this the default for llvm-symbolizer.
-                        "--relative-address",
+                      // Pass --relative-address on Windows so that we don't
+                      // have to add ImageBase from PE file.
+                      // FIXME: Make this the default for llvm-symbolizer.
+                      "--relative-address",
 #endif
-                        "--demangle", nullptr};
+                      "--demangle"};
   int RunResult =
-      sys::ExecuteAndWait(LLVMSymbolizerPath, Args, nullptr, Redirects);
+      sys::ExecuteAndWait(LLVMSymbolizerPath, Args, None, Redirects);
   if (RunResult != 0)
     return false;
 
diff --git a/llvm/lib/Support/Unix/Program.inc b/llvm/lib/Support/Unix/Program.inc
index be971555b82..d0abc3763e8 100644
--- a/llvm/lib/Support/Unix/Program.inc
+++ b/llvm/lib/Support/Unix/Program.inc
@@ -23,6 +23,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/raw_ostream.h"
 #if HAVE_SYS_STAT_H
 #include <sys/stat.h>
@@ -164,8 +165,18 @@ static void SetMemoryLimits(unsigned size) {
 
 }
 
-static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Envp, ArrayRef<Optional<StringRef>> Redirects,
+static std::vector<const char *>
+toNullTerminatedCStringArray(ArrayRef<StringRef> Strings, StringSaver &Saver) {
+  std::vector<const char *> Result;
+  for (StringRef S : Strings)
+    Result.push_back(Saver.save(S).data());
+  Result.push_back(nullptr);
+  return Result;
+}
+
+static bool Execute(ProcessInfo &PI, StringRef Program,
+                    ArrayRef<StringRef> Args, Optional<ArrayRef<StringRef>> Env,
+                    ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg) {
   if (!llvm::sys::fs::exists(Program)) {
     if (ErrMsg)
@@ -174,6 +185,18 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
     return false;
   }
 
+  BumpPtrAllocator Allocator;
+  StringSaver Saver(Allocator);
+  std::vector<const char *> ArgVector, EnvVector;
+  const char **Argv = nullptr;
+  const char **Envp = nullptr;
+  ArgVector = toNullTerminatedCStringArray(Args, Saver);
+  Argv = ArgVector.data();
+  if (Env) {
+    EnvVector = toNullTerminatedCStringArray(*Env, Saver);
+    Envp = EnvVector.data();
+  }
+
   // If this OS has posix_spawn and there is no memory limit being implied, use
   // posix_spawn.  It is more efficient than fork/exec.
 #ifdef HAVE_POSIX_SPAWN
@@ -227,7 +250,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
     // positive.
     pid_t PID = 0;
     int Err = posix_spawn(&PID, Program.str().c_str(), FileActions,
-                          /*attrp*/nullptr, const_cast<char **>(Args),
+                          /*attrp*/ nullptr, const_cast<char **>(Argv),
                           const_cast<char **>(Envp));
 
     if (FileActions)
@@ -280,12 +303,10 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
       // Execute!
       std::string PathStr = Program;
       if (Envp != nullptr)
-        execve(PathStr.c_str(),
-               const_cast<char **>(Args),
+        execve(PathStr.c_str(), const_cast<char **>(Argv),
                const_cast<char **>(Envp));
       else
-        execv(PathStr.c_str(),
-              const_cast<char **>(Args));
+        execv(PathStr.c_str(), const_cast<char **>(Argv));
       // If the execve() failed, we should exit. Follow Unix protocol and
       // return 127 if the executable was not found, and 126 otherwise.
       // Use _exit rather than exit so that atexit functions and static
diff --git a/llvm/lib/Support/Windows/Program.inc b/llvm/lib/Support/Windows/Program.inc
index 19a38c78432..cb68c5b10e5 100644
--- a/llvm/lib/Support/Windows/Program.inc
+++ b/llvm/lib/Support/Windows/Program.inc
@@ -149,15 +149,9 @@ static HANDLE RedirectIO(Optional<StringRef> Path, int fd,
 
 }
 
-static SmallVector<StringRef, 8> buildArgVector(const char **Args) {
-  SmallVector<StringRef, 8> Result;
-  for (unsigned I = 0; Args[I]; ++I)
-    Result.push_back(StringRef(Args[I]));
-  return Result;
-}
-
-static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Envp, ArrayRef<Optional<StringRef>> Redirects,
+static bool Execute(ProcessInfo &PI, StringRef Program,
+                    ArrayRef<StringRef> Args, Optional<ArrayRef<StringRef>> Env,
+                    ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg) {
   if (!sys::fs::can_execute(Program)) {
     if (ErrMsg)
@@ -176,19 +170,18 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
   // Windows wants a command line, not an array of args, to pass to the new
   // process.  We have to concatenate them all, while quoting the args that
   // have embedded spaces (or are empty).
-  auto ArgVector = buildArgVector(Args);
-  std::string Command = flattenWindowsCommandLine(ArgVector);
+  std::string Command = flattenWindowsCommandLine(Args);
 
   // The pointer to the environment block for the new process.
   std::vector<wchar_t> EnvBlock;
 
-  if (Envp) {
+  if (Env) {
     // An environment block consists of a null-terminated block of
     // null-terminated strings. Convert the array of environment variables to
     // an environment block by concatenating them.
-    for (unsigned i = 0; Envp[i]; ++i) {
+    for (const auto E : *Env) {
       SmallVector<wchar_t, MAX_PATH> EnvString;
-      if (std::error_code ec = windows::UTF8ToUTF16(Envp[i], EnvString)) {
+      if (std::error_code ec = windows::UTF8ToUTF16(E, EnvString)) {
         SetLastError(ec.value());
         MakeErrMsg(ErrMsg, "Unable to convert environment variable to UTF-16");
         return false;