Support: Make Wait's SecondsToWait be Optional [NFC]

[arsenm]

Summary

I found the interaction between SecondsToWait and
WaitUntilChildTerminates confusing. Rather than have a boolean to
ignore the value of SecondsToWait, combine these into one Optional
parameter

No idea if the Windows part builds

[aganea]

llvm/include/llvm/Support/Program.h

210

I think the idea is to switch to using std::optional. You can do it now, or someone will do it later.

llvm/unittests/Support/ProgramTest.cpp

282–283

/*SecondsToWait*/ like above?

428

Perhaps /*SecondsToWait*/ here too?

[sepavloff]

llvm/lib/Support/Windows/Program.inc

423

You should put here *SecondsToWait > 0.

[sepavloff]

llvm/lib/Support/Windows/Program.inc

416

if here is excessive.

[arsenm]

Rebase on move to std::optional, address comments

[aganea]

llvm/lib/Support/Program.cpp

35

SecondsToWait here is not completely symmetric with Wait() (value 0 here has the meaning of std::nullopt in Wait), but maybe it's ok. Do you think it's worth changing this to std::optional as well?

llvm/lib/Support/Unix/Program.inc

408

nit: I would add braces here, after else, please see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
"Use braces for the if block to keep it uniform with the else block." (and vice-versa)

[alexfh]

llvm/unittests/Support/ProgramTest.cpp

428

Or rather /*SecondsToWait=*/, since this format is more common and supported by clang-tidy (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html). Same for other argument comments.

[arsenm]

Rename test and address comments

llvm/lib/Support/Program.cpp

35

That would expose both the std::nullopt and SecondsToWait == 0 possibilities, which I'm not sure makes any sense here. If you pass a 0, you're expecting to launch a process and have it immediately complete?

[aganea]

LGTM.

llvm/lib/Support/Program.cpp

35

The annoying thing is that 0 here means the exact opposite as in Wait().
ExecuteAndWait -> 0 means wait forever.
Wait() -> 0 means don't wait at all.

In that sense, yes if we had std::option here and passed 0, that would be (almost) like calling ExecuteNoWait().

Right now we have 6 calling site for ExecuteNoWait() across the monorepo, and 47 for ExecuteAndWait(). Maybe we should merge them into one Execute(). But let's go ahead with your change and we'll do that as a second step? WDYT?

llvm/unittests/Support/ProgramTest.cpp

245

/*Redirects=*/ and /*MemoryLimit=*/

283

/*MemoryLimit=*/

[alexfh]

LGTM modulo open comments.

llvm/unittests/Support/ProgramTest.cpp

428

nit: No space between /*SecondsToWait=*/ and 5. Or better clang-format all changed lines (e.g. using clang-format-diff - see https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).

[arsenm]

15a6e3c636977dc962a415c067182e6d57242116

llvm/lib/Support/Program.cpp

35

They could be merged, though it removes the "simple" option

llvm/include/llvm/Support/Program.h

#endif
   /// This function waits for the process specified by \p PI to finish.
   /// \returns A \see ProcessInfo struct with Pid set to:
   /// \li The process id of the child process if the child process has changed
   /// state.
   /// \li 0 if the child process has not changed state.
   /// \note Users of this function should always check the ReturnCode member of
   /// the \see ProcessInfo returned from this function.
   ProcessInfo Wait(
-      const ProcessInfo &PI,  ///< The child process that should be waited on.
-      unsigned SecondsToWait, ///< 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 function returns. If zero, this function
-      ///< will perform a non-blocking wait on the child process.
-      bool WaitUntilTerminates, ///< If true, ignores \p SecondsToWait and waits
-      ///< until child has terminated.
+      const ProcessInfo &PI, ///< The child process that should be waited on.
+      std::optional<unsigned> SecondsToWait, ///< If std::nullopt, waits until
+      ///< child has terminated.
+      ///< If a value, this specifies the amount of time to wait for the child
+      ///< process to exit. If the time expires, the child is killed and this
+      ///< function returns. If zero, this function will perform a non-blocking
+      ///< wait on the child process.
       std::string *ErrMsg = nullptr, ///< If non-zero, provides a pointer to a
       ///< string instance in which error messages will be returned. If the
       ///< string is non-empty upon return an error occurred while invoking the
       ///< program.
       std::optional<ProcessStatistics> *ProcStat =
           nullptr ///< If non-zero, provides
-      /// a pointer to a structure in which process execution statistics will be
-      /// stored.
+      /// a pointer to a structure in which process execution statistics will
+      /// be stored.
   );
 
   /// Print a command argument, and optionally quote it.
   void printArg(llvm::raw_ostream &OS, StringRef Arg, bool Quote);
 
 #if defined(_WIN32)
   /// Given a list of command line arguments, quote and escape them as necessary
   /// to build a single flat command line appropriate for calling CreateProcess
   /// on
   /// Windows.
   ErrorOr<std::wstring> flattenWindowsCommandLine(ArrayRef<StringRef> Args);
 #endif
   }
 }
 
 #endif

llvm/lib/Support/Program.cpp

static bool Execute(ProcessInfo &PI, StringRef Program,
                     std::optional<ArrayRef<StringRef>> Env,
                     ArrayRef<std::optional<StringRef>> Redirects,
                     unsigned MemoryLimit, std::string *ErrMsg,
                     BitVector *AffinityMask);
 
 int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
                         std::optional<ArrayRef<StringRef>> Env,
                         ArrayRef<std::optional<StringRef>> Redirects,
                         unsigned SecondsToWait, unsigned MemoryLimit,
                         std::string *ErrMsg, bool *ExecutionFailed,
                         std::optional<ProcessStatistics> *ProcStat,
                         BitVector *AffinityMask) {
   assert(Redirects.empty() || Redirects.size() == 3);
   ProcessInfo PI;
   if (Execute(PI, Program, Args, Env, Redirects, MemoryLimit, ErrMsg,
               AffinityMask)) {
     if (ExecutionFailed)
       *ExecutionFailed = false;
-    ProcessInfo Result =
-        Wait(PI, SecondsToWait, /*WaitUntilTerminates=*/SecondsToWait == 0,
-             ErrMsg, ProcStat);
+    ProcessInfo Result = Wait(
+        PI, SecondsToWait == 0 ? std::nullopt : std::optional(SecondsToWait),
     return Result.ReturnCode;
   }
 
   if (ExecutionFailed)
     *ExecutionFailed = true;
 
   return -1;
 }

llvm/lib/Support/Unix/Program.inc

pid_t(llvm::sys::wait4)(pid_t pid, int *status, int options,
 
   // The child has already terminated, so a blocking wait on it is okay in the
   // absence of indiscriminate `wait` calls from the current process (which
   // would cause the call here to fail with ECHILD).
   return ::wait4(pid, status, options & ~WNOHANG, usage);
 }
 #endif
 
-ProcessInfo llvm::sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
-                            bool WaitUntilTerminates, std::string *ErrMsg,
+ProcessInfo llvm::sys::Wait(const ProcessInfo &PI, std::optional<unsigned> SecondsToWait,
+                            std::string *ErrMsg,
                             std::optional<ProcessStatistics> *ProcStat) {
   struct sigaction Act, Old;
   assert(PI.Pid && "invalid pid to wait on, process not started?");
 
   int WaitPidOptions = 0;
   pid_t ChildPid = PI.Pid;
-  if (WaitUntilTerminates) {
-    SecondsToWait = 0;
-  } else if (SecondsToWait) {
+  bool WaitUntilTerminates = false;
+  if (!SecondsToWait) {
+    WaitUntilTerminates = true;
+  } else if (*SecondsToWait != 0) {
     // Install a timeout handler.  The handler itself does nothing, but the
     // simple fact of having a handler at all causes the wait below to return
     // with EINTR, unlike if we used SIG_IGN.
     memset(&Act, 0, sizeof(Act));
     Act.sa_handler = TimeOutHandler;
     sigemptyset(&Act.sa_mask);
     sigaction(SIGALRM, &Act, &Old);
     // FIXME The alarm signal may be delivered to another thread.
-    alarm(SecondsToWait);
-  } else if (SecondsToWait == 0)
+
+    alarm(*SecondsToWait);
+  } else {
     WaitPidOptions = WNOHANG;
+  }
 
   // Parent process: Wait for the child process to terminate.
   int status;
   ProcessInfo WaitResult;
   rusage Info;
   if (ProcStat)
     ProcStat->reset();
 

llvm/lib/Support/Windows/Program.inc

ErrorOr<std::wstring> sys::flattenWindowsCommandLine(ArrayRef<StringRef> Args) {
 
   SmallVector<wchar_t, MAX_PATH> CommandUtf16;
   if (std::error_code ec = windows::UTF8ToUTF16(Command, CommandUtf16))
     return ec;
 
   return std::wstring(CommandUtf16.begin(), CommandUtf16.end());
 }
 
-ProcessInfo sys::Wait(const ProcessInfo &PI, unsigned SecondsToWait,
-                      bool WaitUntilChildTerminates, std::string *ErrMsg,
-                      std::optional<ProcessStatistics> *ProcStat) {
+ProcessInfo sys::Wait(const ProcessInfo &PI, std::optional<unsigned> SecondsToWait,
+                      std::string *ErrMsg, std::optional<ProcessStatistics> *ProcStat) {
   assert(PI.Pid && "invalid pid to wait on, process not started?");
   assert((PI.Process && PI.Process != INVALID_HANDLE_VALUE) &&
          "invalid process handle to wait on, process not started?");
-  DWORD milliSecondsToWait = 0;
-  if (WaitUntilChildTerminates)
-    milliSecondsToWait = INFINITE;
-  else if (SecondsToWait > 0)
-    milliSecondsToWait = SecondsToWait * 1000;
+  DWORD milliSecondsToWait = SecondsToWait ? *SecondsToWait * 1000 : INFINITE;
 
   ProcessInfo WaitResult = PI;
   if (ProcStat)
     ProcStat->reset();
   DWORD WaitStatus = WaitForSingleObject(PI.Process, milliSecondsToWait);
   if (WaitStatus == WAIT_TIMEOUT) {
-    if (SecondsToWait) {
+    if (*SecondsToWait > 0) {
       if (!TerminateProcess(PI.Process, 1)) {
         if (ErrMsg)
           MakeErrMsg(ErrMsg, "Failed to terminate timed-out program");
 
         // -2 indicates a crash or timeout as opposed to failure to execute.
         WaitResult.ReturnCode = -2;
         CloseHandle(PI.Process);
         return WaitResult;

llvm/unittests/Support/ProgramTest.cpp

TEST_F(ProgramEnvTest, CreateProcessTrailingSlash) {
   std::string error;
   bool ExecutionFailed;
   // Redirect stdout and stdin to NUL, but let stderr through.
 #ifdef _WIN32
   StringRef nul("NUL");
 #else
   StringRef nul("/dev/null");
 #endif
-  std::optional<StringRef> redirects[] = {nul, nul, None};
+  std::optional<StringRef> redirects[] = {nul, nul, std::nullopt};
   int rc = ExecuteAndWait(my_exe, argv, getEnviron(), redirects,
                           /*secondsToWait=*/ 10, /*memoryLimit=*/ 0, &error,
                           &ExecutionFailed);
   EXPECT_FALSE(ExecutionFailed) << error;
   EXPECT_EQ(0, rc);
 }
 
 TEST_F(ProgramEnvTest, TestExecuteNoWait) {
TEST_F(ProgramEnvTest, TestExecuteNoWait) {
   bool ExecutionFailed;
   ProcessInfo PI1 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
                                   &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI1.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
   unsigned LoopCount = 0;
 
-  // Test that Wait() with WaitUntilTerminates=true works. In this case,
+  // Test that Wait() with SecondsToWait=std::nullopt works. In this case,
   // LoopCount should only be incremented once.
   while (true) {
     ++LoopCount;
-    ProcessInfo WaitResult = llvm::sys::Wait(PI1, 0, true, &Error);
+    ProcessInfo WaitResult =
+        llvm::sys::Wait(PI1, /*SecondsToWait=*/ std::nullopt, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI1.Pid)
       break;
   }
 
   EXPECT_EQ(LoopCount, 1u) << "LoopCount should be 1";
 
-  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(), {}, 0, &Error,
+  ProcessInfo PI2 = ExecuteNoWait(Executable, argv, getEnviron(),
+                                  /*Redirects*/ {}, /*MemoryLimit*/ 0, &Error,
                                   &ExecutionFailed);
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_NE(PI2.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
   // Test that Wait() with SecondsToWait=0 performs a non-blocking wait. In this
-  // cse, LoopCount should be greater than 1 (more than one increment occurs).
+  // case, LoopCount should be greater than 1 (more than one increment occurs).
   while (true) {
     ++LoopCount;
-    ProcessInfo WaitResult = llvm::sys::Wait(PI2, 0, false, &Error);
+    ProcessInfo WaitResult = llvm::sys::Wait(PI2, /*SecondsToWait=*/ 0, &Error);
     ASSERT_TRUE(Error.empty());
     if (WaitResult.Pid == PI2.Pid)
       break;
   }
 
   ASSERT_GT(LoopCount, 1u) << "LoopCount should be >1";
 }
 
StringRef argv[] = {
       Executable, "--gtest_filter=ProgramEnvTest.TestExecuteAndWaitTimeout"};
 
   // Add LLVM_PROGRAM_TEST_TIMEOUT to the environment of the child.
  addEnvVar("LLVM_PROGRAM_TEST_TIMEOUT=1");
 
   std::string Error;
   bool ExecutionFailed;
   int RetCode =
-      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*secondsToWait=*/1, 0,
-                     &Error, &ExecutionFailed);
+      ExecuteAndWait(Executable, argv, getEnviron(), {}, /*SecondsToWait=*/ 1,
+                     /*MemoryLimit*/ 0, &Error, &ExecutionFailed);
   ASSERT_EQ(-2, RetCode);
 }
 
 TEST(ProgramTest, TestExecuteNegative) {
   std::string Executable = "i_dont_exist";
   StringRef argv[] = {Executable};
 
   {
     std::string Error;
     bool ExecutionFailed;
-    int RetCode = ExecuteAndWait(Executable, argv, llvm::None, {}, 0, 0, &Error,
-                                 &ExecutionFailed);
+    int RetCode = ExecuteAndWait(Executable, argv, std::nullopt, {}, 0, 0,
+                                 &Error, &ExecutionFailed);
     ASSERT_LT(RetCode, 0) << "On error ExecuteAndWait should return 0 or "
                              "positive value indicating the result code";
     ASSERT_TRUE(ExecutionFailed);
     ASSERT_FALSE(Error.empty());
   }
 
   {
     std::string Error;
     bool ExecutionFailed;
-    ProcessInfo PI = ExecuteNoWait(Executable, argv, llvm::None, {}, 0, &Error,
-                                   &ExecutionFailed);
+    ProcessInfo PI = ExecuteNoWait(Executable, argv, std::nullopt, {}, 0,
+                                   &Error, &ExecutionFailed);
     ASSERT_EQ(PI.Pid, ProcessInfo::InvalidPid)
         << "On error ExecuteNoWait should return an invalid ProcessInfo";
     ASSERT_TRUE(ExecutionFailed);
     ASSERT_FALSE(Error.empty());
   }
 
 }
 
TEST_F(ProgramEnvTest, TestLockFile) {
   ASSERT_FALSE(ExecutionFailed) << Error;
   ASSERT_TRUE(Error.empty());
   ASSERT_NE(PI2.Pid, ProcessInfo::InvalidPid) << "Invalid process id";
 
   // Wait some time to give the child process a chance to start.
   std::this_thread::sleep_for(std::chrono::milliseconds(100));
 
   ASSERT_NO_ERROR(fs::unlockFile(FD1));
-  ProcessInfo WaitResult = llvm::sys::Wait(PI2, 5 /* seconds */, true, &Error);
+  ProcessInfo WaitResult = llvm::sys::Wait(PI2, /*SecondsToWait=*/ 5, &Error);
   ASSERT_TRUE(Error.empty());
   ASSERT_EQ(0, WaitResult.ReturnCode);
   ASSERT_EQ(WaitResult.Pid, PI2.Pid);
   sys::fs::remove(LockedFile);
 }
 
 TEST_F(ProgramEnvTest, TestExecuteWithNoStacktraceHandler) {
   using namespace llvm::sys;