[libc] [UnitTest] Add timeout to death tests

[abrachet]

Summary

This patch adds a timeout of 500ms to death tests. As we add multithreaded code and locks, deadlocks become more likely so timeout will be useful.

Additionally:

• Better error handling in invokeSubprocess
• Makes ProcessStatus's methods const

[MaskRay]

libc/utils/testutils/ExecuteFunction.h

25

I think for new projects, we don't have to use VariableName. We should use the ideal case variableName.

https://llvm.org/docs/Proposals/VariableNames.html Everyone agrees that the current VariableName convention is broken. People have concerns with existing code base because renaming can clutter up the history. But for a new project, there is simply not meaningful to start with VariableName.

[abrachet]

libc/utils/testutils/ExecuteFunction.h

25

Ok I will change the variables that I created in this patch. Did Rui upstream the tool he used to convert lld's code base so that I could use it here?

[abrachet]

Made variables added by this patch start with lower case

[PaulkaToast]

libc/utils/testutils/ExecuteFunctionUnix.cpp

64

I think it'd be useful to include the length of the timeout in the error message.
e.g "Process timed out after 500ms"

[abrachet]

Added ProcessStatus::timedOut() and better error messages in Test::testProcessExits/Killed

[PaulkaToast]

This looks ready to me.
nit: I'd maybe add some comments explaining the pipe trick since it might not be overtly obvious what's happening here on first glance. (:

[abrachet]

Add comment.

libc/utils/UnitTest/Test.cpp

bool Test::testStrNe(RunContext &Ctx, const char *LHS, const char *RHS,
                      unsigned long Line) {
   return internal::test(Ctx, Cond_NE, llvm::StringRef(LHS),
                         llvm::StringRef(RHS), LHSStr, RHSStr, File, Line);
 }
 
 bool Test::testProcessKilled(RunContext &Ctx, testutils::FunctionCaller *Func,
                              int Signal, const char *LHSStr, const char *RHSStr,
                              const char *File, unsigned long Line) {
-  testutils::ProcessStatus Result = testutils::invokeInSubprocess(Func);
+  testutils::ProcessStatus Result = testutils::invokeInSubprocess(Func, 500);
+
+  if (const char *Error = Result.getError()) {
+    Ctx.markFail();
+    llvm::outs() << File << ":" << Line << ": FAILURE\n" << Error << '\n';
+    return false;
+  }
 
   if (Result.exitedNormally()) {
     Ctx.markFail();
     llvm::outs() << File << ":" << Line << ": FAILURE\n"
                  << "Expected " << LHSStr
                  << " to be killed by a signal\nBut it exited normally!\n";
     return false;
   }
llvm::outs() << File << ":" << Line << ": FAILURE\n"
                << '\n';
   return false;
 }
 
 bool Test::testProcessExits(RunContext &Ctx, testutils::FunctionCaller *Func,
                             int ExitCode, const char *LHSStr,
                             const char *RHSStr, const char *File,
                             unsigned long Line) {
-  testutils::ProcessStatus Result = testutils::invokeInSubprocess(Func);
+  testutils::ProcessStatus Result = testutils::invokeInSubprocess(Func, 500);
+
+  if (const char *Error = Result.getError()) {
+    Ctx.markFail();
+    llvm::outs() << File << ":" << Line << ": FAILURE\n" << Error << '\n';
+    return false;
+  }
 
   if (!Result.exitedNormally()) {
     Ctx.markFail();
     llvm::outs() << File << ":" << Line << ": FAILURE\n"
                  << "Expected " << LHSStr << '\n'
                  << "to exit with exit code " << ExitCode << '\n'
                  << "But it exited abnormally!\n";
     return false;

libc/utils/testutils/ExecuteFunction.h

 class FunctionCaller {
 public:
   virtual ~FunctionCaller() {}
   virtual void operator()() = 0;
 };
 
 struct ProcessStatus {
   int PlatformDefined;
+  const char *Failure = nullptr;
 
-  bool exitedNormally();
-  int getExitCode();
-  int getFatalSignal();
+  static ProcessStatus Error(const char *Error) { return {0, Error}; }
+
+  const char *getError() const { return Failure; }
+  bool exitedNormally() const;
+  int getExitCode() const;
+  int getFatalSignal() const;
 };
 
-ProcessStatus invokeInSubprocess(FunctionCaller *Func);
+ProcessStatus invokeInSubprocess(FunctionCaller *Func, unsigned TimeoutMS = -1);
 
 const char *signalAsString(int Signum);
 
 } // namespace testutils
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_UTILS_TESTUTILS_EXECUTEFUNCTION_H

libc/utils/testutils/ExecuteFunctionUnix.cpp

 //===------- ExecuteFunction implementation for Unix-like Systems ---------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "ExecuteFunction.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstdlib>
+#include <memory>
+#include <poll.h>
 #include <signal.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 namespace __llvm_libc {
 namespace testutils {
 
-bool ProcessStatus::exitedNormally() { return WIFEXITED(PlatformDefined); }
+bool ProcessStatus::exitedNormally() const {
+  return WIFEXITED(PlatformDefined);
+}
 
-int ProcessStatus::getExitCode() {
+int ProcessStatus::getExitCode() const {
   assert(exitedNormally() && "Abnormal termination, no exit code");
   return WEXITSTATUS(PlatformDefined);
 }
 
-int ProcessStatus::getFatalSignal() {
+int ProcessStatus::getFatalSignal() const {
   if (exitedNormally())
     return 0;
   return WTERMSIG(PlatformDefined);
 }
 
-ProcessStatus invokeInSubprocess(FunctionCaller *Func) {
+ProcessStatus invokeInSubprocess(FunctionCaller *Func, unsigned TimeoutMS) {
+  std::unique_ptr<FunctionCaller> X(Func);
+  int PipeFDs[2];
+  if (::pipe(PipeFDs) == -1)
+    return ProcessStatus::Error("pipe(2) failed");
+
   // Don't copy the buffers into the child process and print twice.
   llvm::outs().flush();
   llvm::errs().flush();
   pid_t Pid = ::fork();
+  if (Pid == -1)
+    return ProcessStatus::Error("fork(2) failed");
+
   if (!Pid) {
     (*Func)();
     std::exit(0);
   }
+  ::close(PipeFDs[1]);
+
+  struct pollfd PollFD {
+    PipeFDs[0], 0, 0
+  };
+  if (::poll(&PollFD, 1, TimeoutMS) == -1)
+    return ProcessStatus::Error("poll(2) failed");
+  // If the pipe wasn't closed by the child yet then timeout has expired.
+  if (!(PollFD.revents & POLLHUP)) {
+    ::kill(Pid, SIGKILL);
+    return ProcessStatus::Error("Process timed out");
+  }
 
-  int WStatus;
-  ::waitpid(Pid, &WStatus, 0);
-  delete Func;
+  int WStatus = 0;
+  int Status = ::waitpid(Pid, &WStatus, WNOHANG);
+  assert(Status == Pid && "wait call should not block here");
+  (void)Status;
   return {WStatus};
 }
 
 const char *signalAsString(int Signum) { return ::strsignal(Signum); }
 
 } // namespace testutils
 } // namespace __llvm_libc