Convenience/safety fix for llvm::sys::Execute(And|No)Wait

Summary:
Change the type of the Redirects parameter of llvm::sys::ExecuteAndWait,
ExecuteNoWait and other APIs that wrap them from `const StringRef **` to
`ArrayRef<Optional<StringRef>>`, which is safer and simplifies the use of these
APIs (no more local StringRef variables just to get a pointer to).

Corresponding clang changes will be posted as a separate patch.

Reviewers: bkramer

Reviewed By: bkramer

Subscribers: vsk, llvm-commits

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@313155 91177308-0d34-0410-b5e6-96231b3b80d8

Author: alexfh

include/llvm/Support/Program.h

@@ -15,6 +15,7 @@
 #define LLVM_SUPPORT_PROGRAM_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/ErrorOr.h"
 #include <system_error>
 
@@ -69,7 +70,7 @@ struct ProcessInfo {
   /// \returns The fully qualified path to the first \p Name in \p Paths if it
   ///   exists. \p Name if \p Name has slashes in it. Otherwise an error.
   ErrorOr<std::string>
-  findProgramByName(StringRef Name, ArrayRef<StringRef> Paths = None);
+  findProgramByName(StringRef Name, ArrayRef<StringRef> Paths = {});
 
   // These functions change the specified standard stream (stdin or stdout) to
   // binary mode. They return errc::success if the specified stream
@@ -84,7 +85,7 @@ struct ProcessInfo {
   /// This function waits for the program to finish, so should be avoided in
   /// library functions that aren't expected to block. Consider using
   /// ExecuteNoWait() instead.
-  /// @returns an integer result code indicating the status of the program.
+  /// \returns an integer result code indicating the status of the program.
   /// A zero or positive value indicates the result code of the program.
   /// -1 indicates failure to execute
   /// -2 indicates a crash during execution or timeout
@@ -97,14 +98,14 @@ struct ProcessInfo {
       const char **Env = nullptr, ///< An optional vector of strings to use for
       ///< the program's environment. If not provided, the current program's
       ///< environment will be used.
-      const StringRef **Redirects = nullptr, ///< An optional array of pointers
-      ///< to paths. If the array is null, no redirection is done. The array
-      ///< should have a size of at least three. The inferior process's
-      ///< stdin(0), stdout(1), and stderr(2) will be redirected to the
-      ///< corresponding paths.
-      ///< When an empty path is passed in, the corresponding file
-      ///< descriptor will be disconnected (ie, /dev/null'd) in a portable
-      ///< way.
+      ArrayRef<Optional<StringRef>> Redirects = {}, ///<
+      ///< An array of optional paths. Should have a size of zero or three.
+      ///< If the array is empty, no redirections are performed.
+      ///< Otherwise, the inferior process's stdin(0), stdout(1), and stderr(2)
+      ///< will be redirected to the corresponding paths, if the optional path
+      ///< is present (not \c llvm::None).
+      ///< When an empty path is passed in, the corresponding file descriptor
+      ///< will be disconnected (ie, /dev/null'd) in a portable way.
       unsigned SecondsToWait = 0, ///< If non-zero, this specifies the amount
       ///< of time to wait for the child process to exit. If the time
       ///< expires, the child is killed and this call returns. If zero,
@@ -122,12 +123,12 @@ struct ProcessInfo {
 
   /// Similar to ExecuteAndWait, but returns immediately.
   /// @returns The \see ProcessInfo of the newly launced process.
-  /// \note On Microsoft Windows systems, users will need to either call \see
-  /// Wait until the process finished execution or win32 CloseHandle() API on
-  /// ProcessInfo.ProcessHandle to avoid memory leaks.
+  /// \note On Microsoft Windows systems, users will need to either call
+  /// \see Wait until the process finished execution or win32 CloseHandle() API
+  /// on ProcessInfo.ProcessHandle to avoid memory leaks.
   ProcessInfo ExecuteNoWait(StringRef Program, const char **Args,
                             const char **Env = nullptr,
-                            const StringRef **Redirects = nullptr,
+                            ArrayRef<Optional<StringRef>> Redirects = {},
                             unsigned MemoryLimit = 0,
                             std::string *ErrMsg = nullptr,
                             bool *ExecutionFailed = nullptr);

lib/Support/GraphWriter.cpp

@@ -96,15 +96,15 @@ static bool ExecGraphViewer(StringRef ExecPath, std::vector<const char *> &args,
                             std::string &ErrMsg) {
   assert(args.back() == nullptr);
   if (wait) {
-    if (sys::ExecuteAndWait(ExecPath, args.data(), nullptr, nullptr, 0, 0,
+    if (sys::ExecuteAndWait(ExecPath, args.data(), nullptr, {}, 0, 0,
                             &ErrMsg)) {
       errs() << "Error: " << ErrMsg << "\n";
       return true;
     }
     sys::fs::remove(Filename);
     errs() << " done. \n";
   } else {
-    sys::ExecuteNoWait(ExecPath, args.data(), nullptr, nullptr, 0, &ErrMsg);
+    sys::ExecuteNoWait(ExecPath, args.data(), nullptr, {}, 0, &ErrMsg);
     errs() << "Remember to erase graph file: " << Filename << "\n";
   }
   return false;

lib/Support/Program.cpp

@@ -24,13 +24,14 @@ using namespace sys;
 //===----------------------------------------------------------------------===//
 
 static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Env, const StringRef **Redirects,
+                    const char **Env, ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg);
 
 int sys::ExecuteAndWait(StringRef Program, const char **Args, const char **Envp,
-                        const StringRef **Redirects, unsigned SecondsToWait,
-                        unsigned MemoryLimit, std::string *ErrMsg,
-                        bool *ExecutionFailed) {
+                        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 (ExecutionFailed)
@@ -47,9 +48,11 @@ int sys::ExecuteAndWait(StringRef Program, const char **Args, const char **Envp,
 }
 
 ProcessInfo sys::ExecuteNoWait(StringRef Program, const char **Args,
-                               const char **Envp, const StringRef **Redirects,
+                               const char **Envp,
+                               ArrayRef<Optional<StringRef>> Redirects,
                                unsigned MemoryLimit, std::string *ErrMsg,
                                bool *ExecutionFailed) {
+  assert(Redirects.empty() || Redirects.size() == 3);
   ProcessInfo PI;
   if (ExecutionFailed)
     *ExecutionFailed = false;

lib/Support/Signals.cpp

@@ -123,11 +123,7 @@ static bool printSymbolizedStackTrace(StringRef Argv0,
     }
   }
 
-  StringRef InputFileStr(InputFile);
-  StringRef OutputFileStr(OutputFile);
-  StringRef StderrFileStr;
-  const StringRef *Redirects[] = {&InputFileStr, &OutputFileStr,
-                                  &StderrFileStr};
+  Optional<StringRef> Redirects[] = {InputFile.str(), OutputFile.str(), llvm::None};
   const char *Args[] = {"llvm-symbolizer", "--functions=linkage", "--inlining",
 #ifdef LLVM_ON_WIN32
                         // Pass --relative-address on Windows so that we don't

lib/Support/Unix/Program.inc

@@ -93,7 +93,7 @@ ErrorOr<std::string> sys::findProgramByName(StringRef Name,
   return errc::no_such_file_or_directory;
 }
 
-static bool RedirectIO(const StringRef *Path, int FD, std::string* ErrMsg) {
+static bool RedirectIO(Optional<StringRef> Path, int FD, std::string* ErrMsg) {
   if (!Path) // Noop
     return false;
   std::string File;
@@ -165,7 +165,7 @@ static void SetMemoryLimits(unsigned size) {
 }
 
 static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Envp, const StringRef **Redirects,
+                    const char **Envp, ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg) {
   if (!llvm::sys::fs::exists(Program)) {
     if (ErrMsg)
@@ -186,7 +186,8 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
     // so we copy any StringRefs into this variable.
     std::string RedirectsStorage[3];
 
-    if (Redirects) {
+    if (!Redirects.empty()) {
+      assert(Redirects.size() == 3);
       std::string *RedirectsStr[3] = {nullptr, nullptr, nullptr};
       for (int I = 0; I < 3; ++I) {
         if (Redirects[I]) {
@@ -202,8 +203,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
       if (RedirectIO_PS(RedirectsStr[0], 0, ErrMsg, FileActions) ||
           RedirectIO_PS(RedirectsStr[1], 1, ErrMsg, FileActions))
         return false;
-      if (Redirects[1] == nullptr || Redirects[2] == nullptr ||
-          *Redirects[1] != *Redirects[2]) {
+      if (!Redirects[1] || !Redirects[2] || *Redirects[1] != *Redirects[2]) {
         // Just redirect stderr
         if (RedirectIO_PS(RedirectsStr[2], 2, ErrMsg, FileActions))
           return false;
@@ -253,7 +253,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
     // Child process: Execute the program.
     case 0: {
       // Redirect file descriptors...
-      if (Redirects) {
+      if (!Redirects.empty()) {
         // Redirect stdin
         if (RedirectIO(Redirects[0], 0, ErrMsg)) { return false; }
         // Redirect stdout

lib/Support/Windows/Program.inc

@@ -103,9 +103,10 @@ ErrorOr<std::string> sys::findProgramByName(StringRef Name,
   return std::string(U8Result.begin(), U8Result.end());
 }
 
-static HANDLE RedirectIO(const StringRef *Path, int fd, std::string* ErrMsg) {
+static HANDLE RedirectIO(Optional<StringRef> Path, int fd,
+                         std::string *ErrMsg) {
   HANDLE h;
-  if (Path == 0) {
+  if (!Path) {
     if (!DuplicateHandle(GetCurrentProcess(), (HANDLE)_get_osfhandle(fd),
                          GetCurrentProcess(), &h,
                          0, TRUE, DUPLICATE_SAME_ACCESS))
@@ -249,7 +250,7 @@ static std::unique_ptr<char[]> flattenArgs(const char **Args) {
 }
 
 static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
-                    const char **Envp, const StringRef **Redirects,
+                    const char **Envp, ArrayRef<Optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg) {
   if (!sys::fs::can_execute(Program)) {
     if (ErrMsg)
@@ -299,7 +300,7 @@ static bool Execute(ProcessInfo &PI, StringRef Program, const char **Args,
   si.hStdOutput = INVALID_HANDLE_VALUE;
   si.hStdError = INVALID_HANDLE_VALUE;
 
-  if (Redirects) {
+  if (!Redirects.empty()) {
     si.dwFlags = STARTF_USESTDHANDLES;
 
     si.hStdInput = RedirectIO(Redirects[0], 0, ErrMsg);

tools/bugpoint/OptimizerDriver.cpp

@@ -230,13 +230,15 @@ bool BugDriver::runPasses(Module *Program,
         << " " << Args[i];
         errs() << "\n";);
 
-  // Redirect stdout and stderr to nowhere if SilencePasses is given
-  StringRef Nowhere;
-  const StringRef *Redirects[3] = {nullptr, &Nowhere, &Nowhere};
+  Optional<StringRef> Redirects[3] = {None, None, None};
+  // Redirect stdout and stderr to nowhere if SilencePasses is given.
+  if (SilencePasses) {
+    Redirects[1] = "";
+    Redirects[2] = "";
+  }
 
   std::string ErrMsg;
-  int result = sys::ExecuteAndWait(Prog, Args.data(), nullptr,
-                                   (SilencePasses ? Redirects : nullptr),
+  int result = sys::ExecuteAndWait(Prog, Args.data(), nullptr, Redirects,
                                    Timeout, MemoryLimit, &ErrMsg);
 
   // If we are supposed to delete the bitcode file or if the passes crashed,

tools/bugpoint/ToolRunner.cpp

@@ -58,7 +58,7 @@ static int RunProgramWithTimeout(StringRef ProgramPath, const char **Args,
                                  StringRef StdErrFile, unsigned NumSeconds = 0,
                                  unsigned MemoryLimit = 0,
                                  std::string *ErrMsg = nullptr) {
-  const StringRef *Redirects[3] = {&StdInFile, &StdOutFile, &StdErrFile};
+  Optional<StringRef> Redirects[3] = {StdInFile, StdOutFile, StdErrFile};
   return sys::ExecuteAndWait(ProgramPath, Args, nullptr, Redirects, NumSeconds,
                              MemoryLimit, ErrMsg);
 }
@@ -75,7 +75,7 @@ static int RunProgramRemotelyWithTimeout(StringRef RemoteClientPath,
                                          StringRef StdErrFile,
                                          unsigned NumSeconds = 0,
                                          unsigned MemoryLimit = 0) {
-  const StringRef *Redirects[3] = {&StdInFile, &StdOutFile, &StdErrFile};
+  Optional<StringRef> Redirects[3] = {StdInFile, StdOutFile, StdErrFile};
 
   // Run the program remotely with the remote client
   int ReturnCode = sys::ExecuteAndWait(RemoteClientPath, Args, nullptr,

tools/dsymutil/MachOUtils.cpp

@@ -45,7 +45,7 @@ static bool runLipo(StringRef SDKPath, SmallVectorImpl<const char *> &Args) {
 
   std::string ErrMsg;
   int result =
-      sys::ExecuteAndWait(*Path, Args.data(), nullptr, nullptr, 0, 0, &ErrMsg);
+      sys::ExecuteAndWait(*Path, Args.data(), nullptr, {}, 0, 0, &ErrMsg);
   if (result) {
     errs() << "error: lipo: " << ErrMsg << "\n";
     return false;

tools/llvm-cov/CodeCoverage.cpp

@@ -458,10 +458,7 @@ void CodeCoverageTool::demangleSymbols(const CoverageMapping &Coverage) {
   for (const std::string &Arg : ViewOpts.DemanglerOpts)
     ArgsV.push_back(Arg.c_str());
   ArgsV.push_back(nullptr);
-  StringRef InputPathRef = InputPath.str();
-  StringRef OutputPathRef = OutputPath.str();
-  StringRef StderrRef;
-  const StringRef *Redirects[] = {&InputPathRef, &OutputPathRef, &StderrRef};
+  Optional<StringRef> Redirects[] = {InputPath.str(), OutputPath.str(), {""}};
   std::string ErrMsg;
   int RC = sys::ExecuteAndWait(ViewOpts.DemanglerOpts[0], ArgsV.data(),
                                /*env=*/nullptr, Redirects, /*secondsToWait=*/0,

unittests/Support/ProgramTest.cpp

@@ -145,11 +145,10 @@ TEST_F(ProgramEnvTest, CreateProcessLongPath) {
   LongPath.push_back('\\');
   // MAX_PATH = 260
   LongPath.append(260 - TestDirectory.size(), 'a');
-  StringRef LongPathRef(LongPath);
 
   std::string Error;
   bool ExecutionFailed;
-  const StringRef *Redirects[] = { nullptr, &LongPathRef, nullptr };
+  Optional<StringRef> Redirects[] = {None, LongPath.str(), None};
   int RC = ExecuteAndWait(MyExe, ArgV, getEnviron(), Redirects,
     /*secondsToWait=*/ 10, /*memoryLimit=*/ 0, &Error,
     &ExecutionFailed);
@@ -192,7 +191,7 @@ TEST_F(ProgramEnvTest, CreateProcessTrailingSlash) {
 #else
   StringRef nul("/dev/null");
 #endif
-  const StringRef *redirects[] = { &nul, &nul, nullptr };
+  Optional<StringRef> redirects[] = { nul, nul, None };
   int rc = ExecuteAndWait(my_exe, argv, getEnviron(), redirects,
                           /*secondsToWait=*/ 10, /*memoryLimit=*/ 0, &error,
                           &ExecutionFailed);
@@ -221,8 +220,8 @@ TEST_F(ProgramEnvTest, TestExecuteNoWait) {
 
   std::string Error;
   bool ExecutionFailed;
-  ProcessInfo PI1 = ExecuteNoWait(Executable, argv, getEnviron(), nullptr, 0,
-                                  &Error, &ExecutionFailed);
+  ProcessInfo PI1 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
+                                  &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI1.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
@@ -240,8 +239,8 @@ TEST_F(ProgramEnvTest, TestExecuteNoWait) {
 
   EXPECT_EQ(LoopCount, 1u) << "LoopCount should be 1";
 
-  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), nullptr, 0,
-                                  &Error, &ExecutionFailed);
+  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
+                                  &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI2.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
@@ -280,7 +279,7 @@ TEST_F(ProgramEnvTest, TestExecuteAndWaitTimeout) {
   std::string Error;
   bool ExecutionFailed;
   int RetCode =
-      ExecuteAndWait(Executable, argv, getEnviron(), nullptr, /*secondsToWait=*/1, 0,
+      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*secondsToWait=*/1, 0,
                      &Error, &ExecutionFailed);
   ASSERT_EQ(-2, RetCode);
 }
@@ -292,8 +291,8 @@ TEST(ProgramTest, TestExecuteNegative) {
   {
     std::string Error;
     bool ExecutionFailed;
-    int RetCode = ExecuteAndWait(Executable, argv, nullptr, nullptr, 0, 0,
-                                 &Error, &ExecutionFailed);
+    int RetCode = ExecuteAndWait(Executable, argv, nullptr, {}, 0, 0, &Error,
+                                 &ExecutionFailed);
     ASSERT_TRUE(RetCode < 0) << "On error ExecuteAndWait should return 0 or "
                                 "positive value indicating the result code";
     ASSERT_TRUE(ExecutionFailed);
@@ -303,8 +302,8 @@ TEST(ProgramTest, TestExecuteNegative) {
   {
     std::string Error;
     bool ExecutionFailed;
-    ProcessInfo PI = ExecuteNoWait(Executable, argv, nullptr, nullptr, 0,
-                                   &Error, &ExecutionFailed);
+    ProcessInfo PI = ExecuteNoWait(Executable, argv, nullptr, {}, 0, &Error,
+                                   &ExecutionFailed);
     ASSERT_EQ(PI.Pid, ProcessInfo::InvalidPid)
         << "On error ExecuteNoWait should return an invalid ProcessInfo";
     ASSERT_TRUE(ExecutionFailed);

utils/not/not.cpp

@@ -39,8 +39,7 @@ int main(int argc, const char **argv) {
   }
 
   std::string ErrMsg;
-  int Result = sys::ExecuteAndWait(*Program, argv, nullptr, nullptr, 0, 0,
-                                   &ErrMsg);
+  int Result = sys::ExecuteAndWait(*Program, argv, nullptr, {}, 0, 0, &ErrMsg);
 #ifdef _WIN32
   // Handle abort() in msvcrt -- It has exit code as 3.  abort(), aka
   // unreachable, should be recognized as a crash.  However, some binaries use