Support: Fix program error test failures when using fork

[arsenm]

The actual exec error will not be immediate in the parent when
using the fork path. The error will manifest on the wait. The
ExecuteNoWait test was also expecting the immediate wait.

Remove the check in the AndWait test check for ExecutionFailed.
The execution did succeed in some sense.

Fixes #129208

[arsenm]

• Support: Fix program error test failures when using fork #129252 👈 (View in Graphite)
• unittests: Use EXPECT_ instead of ASSERT_ in a few tests #129251
• llvm-rc: Relax error message checked in test #129243
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Matt Arsenault (arsenm)

Changes

The actual exec error will not be immediate in the parent when
using the fork path. The error will manifest on the wait. The
ExecuteNoWait test was also expecting the immediate wait.

I hacked up the exit value ExecutionFailed value in ExecuteAndWait,
although arguably the previous behavior was correct. I'm not sure
what the point of the argument is, it's redundant with the return code.

Fixes #129208

---

Full diff: https://github.com/llvm/llvm-project/pull/129252.diff

2 Files Affected:

• (modified) llvm/lib/Support/Program.cpp (+4)
• (modified) llvm/unittests/Support/ProgramTest.cpp (+13-4)

diff --git a/llvm/lib/Support/Program.cpp b/llvm/lib/Support/Program.cpp
index 181f68cfbb8c3..c8208e4be3175 100644
--- a/llvm/lib/Support/Program.cpp
+++ b/llvm/lib/Support/Program.cpp
@@ -45,6 +45,10 @@ int sys::ExecuteAndWait(StringRef Program, ArrayRef<StringRef> Args,
     ProcessInfo Result = Wait(
         PI, SecondsToWait == 0 ? std::nullopt : std::optional(SecondsToWait),
         ErrMsg, ProcStat);
+
+    if (ExecutionFailed && Result.ReturnCode < 0)
+      *ExecutionFailed = true;
+
     return Result.ReturnCode;
   }
 
diff --git a/llvm/unittests/Support/ProgramTest.cpp b/llvm/unittests/Support/ProgramTest.cpp
index 47d2e269afe94..ccb154f7fab1a 100644
--- a/llvm/unittests/Support/ProgramTest.cpp
+++ b/llvm/unittests/Support/ProgramTest.cpp
@@ -433,10 +433,19 @@ TEST(ProgramTest, TestExecuteNegative) {
     bool ExecutionFailed;
     ProcessInfo PI = ExecuteNoWait(Executable, argv, std::nullopt, {}, 0,
                                    &Error, &ExecutionFailed);
-    EXPECT_EQ(PI.Pid, ProcessInfo::InvalidPid)
-        << "On error ExecuteNoWait should return an invalid ProcessInfo";
-    EXPECT_TRUE(ExecutionFailed);
-    EXPECT_FALSE(Error.empty());
+
+    if (ExecutionFailed) {
+      EXPECT_EQ(PI.Pid, ProcessInfo::InvalidPid)
+          << "On error ExecuteNoWait should return an invalid ProcessInfo";
+      EXPECT_FALSE(Error.empty());
+    } else {
+      std::string WaitErrMsg;
+      EXPECT_NE(PI.Pid, ProcessInfo::InvalidPid);
+      ProcessInfo WaitPI = Wait(PI, std::nullopt, &WaitErrMsg);
+      EXPECT_EQ(WaitPI.Pid, PI.Pid);
+      EXPECT_LT(WaitPI.ReturnCode, 0);
+      EXPECT_FALSE(WaitErrMsg.empty());
+    }
   }
 
 }

[MaskRay]

I hacked up the exit value ExecutionFailed value in ExecuteAndWait, although arguably the previous behavior was correct. I'm not sure what the point of the argument is, it's redundant with the return code.

I also feel that the current behavior is better. When we have ReturnCode, it doesn't add value to set ExecutionFailed.
The PRes that adjust the tests look good to me.

[aleks-tmb]

Hi @arsenm, are you planning to merge the fix soon, or would it be better to disable the test in our local testing for now?