Reland "[llvm-exegesis] Refactor parent code to separate function (#86232)"

boomanaiden154 · boomanaiden154 · commit 36a6afdd2c7f · 2024-03-22T10:30:42.000-07:00
This reverts commit c3a41aa. This relands commit bd49375. Apparently I forgot to update a couple values, so this change failed on every builder that builds those sections (should be every Linux platform) rather than something architecture specific like originally thought. I swore I updated the values and ran check-llvm before merging. Wondering If I forgot to push those changes...
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -278,90 +278,51 @@ class SubProcessFunctionExecutorImpl
     return FD;
   }
 
-  Error createSubProcessAndRunBenchmark(
-      StringRef CounterName, SmallVectorImpl<int64_t> &CounterValues,
-      ArrayRef<const char *> ValidationCounters,
-      SmallVectorImpl<int64_t> &ValidationCounterValues) const {
-    int PipeFiles[2];
-    int PipeSuccessOrErr = socketpair(AF_UNIX, SOCK_DGRAM, 0, PipeFiles);
-    if (PipeSuccessOrErr != 0) {
-      return make_error<Failure>(
-          "Failed to create a pipe for interprocess communication between "
-          "llvm-exegesis and the benchmarking subprocess: " +
-          Twine(strerror(errno)));
-    }
-
-    SubprocessMemory SPMemory;
-    Error MemoryInitError = SPMemory.initializeSubprocessMemory(getpid());
-    if (MemoryInitError)
-      return MemoryInitError;
-
-    Error AddMemDefError =
-        SPMemory.addMemoryDefinition(Key.MemoryValues, getpid());
-    if (AddMemDefError)
-      return AddMemDefError;
-
-    pid_t ParentOrChildPID = fork();
-
-    if (ParentOrChildPID == -1) {
-      return make_error<Failure>("Failed to create child process: " +
-                                 Twine(strerror(errno)));
-    }
-
-    if (ParentOrChildPID == 0) {
-      // We are in the child process, close the write end of the pipe.
-      close(PipeFiles[1]);
-      // Unregister handlers, signal handling is now handled through ptrace in
-      // the host process.
-      sys::unregisterHandlers();
-      prepareAndRunBenchmark(PipeFiles[0], Key);
-      // The child process terminates in the above function, so we should never
-      // get to this point.
-      llvm_unreachable("Child process didn't exit when expected.");
-    }
-
+  Error
+  runParentProcess(pid_t ChildPID, int WriteFD, StringRef CounterName,
+                   SmallVectorImpl<int64_t> &CounterValues,
+                   ArrayRef<const char *> ValidationCounters,
+                   SmallVectorImpl<int64_t> &ValidationCounterValues) const {
     const ExegesisTarget &ET = State.getExegesisTarget();
-    auto CounterOrError = ET.createCounter(
-        CounterName, State, ValidationCounters, ParentOrChildPID);
+    auto CounterOrError =
+        ET.createCounter(CounterName, State, ValidationCounters, ChildPID);
 
     if (!CounterOrError)
       return CounterOrError.takeError();
 
     pfm::CounterGroup *Counter = CounterOrError.get().get();
 
-    close(PipeFiles[0]);
-
     // Make sure to attach to the process (and wait for the sigstop to be
     // delivered and for the process to continue) before we write to the counter
     // file descriptor. Attaching to the process before writing to the socket
     // ensures that the subprocess at most has blocked on the read call. If we
     // attach afterwards, the subprocess might exit before we get to the attach
     // call due to effects like scheduler contention, introducing transient
     // failures.
-    if (ptrace(PTRACE_ATTACH, ParentOrChildPID, NULL, NULL) != 0)
+    if (ptrace(PTRACE_ATTACH, ChildPID, NULL, NULL) != 0)
       return make_error<Failure>("Failed to attach to the child process: " +
                                  Twine(strerror(errno)));
 
-    if (waitpid(ParentOrChildPID, NULL, 0) == -1) {
+    if (waitpid(ChildPID, NULL, 0) == -1) {
       return make_error<Failure>(
           "Failed to wait for child process to stop after attaching: " +
           Twine(strerror(errno)));
     }
 
-    if (ptrace(PTRACE_CONT, ParentOrChildPID, NULL, NULL) != 0)
+    if (ptrace(PTRACE_CONT, ChildPID, NULL, NULL) != 0)
       return make_error<Failure>(
           "Failed to continue execution of the child process: " +
           Twine(strerror(errno)));
 
     int CounterFileDescriptor = Counter->getFileDescriptor();
     Error SendError =
-        sendFileDescriptorThroughSocket(PipeFiles[1], CounterFileDescriptor);
+        sendFileDescriptorThroughSocket(WriteFD, CounterFileDescriptor);
 
     if (SendError)
       return SendError;
 
     int ChildStatus;
-    if (waitpid(ParentOrChildPID, &ChildStatus, 0) == -1) {
+    if (waitpid(ChildPID, &ChildStatus, 0) == -1) {
       return make_error<Failure>(
           "Waiting for the child process to complete failed: " +
           Twine(strerror(errno)));
@@ -395,8 +356,7 @@ class SubProcessFunctionExecutorImpl
 
     // An error was encountered running the snippet, process it
     siginfo_t ChildSignalInfo;
-    if (ptrace(PTRACE_GETSIGINFO, ParentOrChildPID, NULL, &ChildSignalInfo) ==
-        -1) {
+    if (ptrace(PTRACE_GETSIGINFO, ChildPID, NULL, &ChildSignalInfo) == -1) {
       return make_error<Failure>("Getting signal info from the child failed: " +
                                  Twine(strerror(errno)));
     }
@@ -405,13 +365,13 @@ class SubProcessFunctionExecutorImpl
     // handlers to run, and calling SIGTERM would mean that ptrace will force
     // it to block in the signal-delivery-stop for the SIGSEGV/other signals,
     // and upon exit.
-    if (kill(ParentOrChildPID, SIGKILL) == -1)
+    if (kill(ChildPID, SIGKILL) == -1)
       return make_error<Failure>("Failed to kill child benchmarking proces: " +
                                  Twine(strerror(errno)));
 
     // Wait for the process to exit so that there are no zombie processes left
     // around.
-    if (waitpid(ParentOrChildPID, NULL, 0) == -1)
+    if (waitpid(ChildPID, NULL, 0) == -1)
       return make_error<Failure>("Failed to wait for process to die: " +
                                  Twine(strerror(errno)));
 
@@ -422,15 +382,65 @@ class SubProcessFunctionExecutorImpl
     return make_error<SnippetSignal>(ChildSignalInfo.si_signo);
   }
 
+  Error createSubProcessAndRunBenchmark(
+      StringRef CounterName, SmallVectorImpl<int64_t> &CounterValues,
+      ArrayRef<const char *> ValidationCounters,
+      SmallVectorImpl<int64_t> &ValidationCounterValues) const {
+    int PipeFiles[2];
+    int PipeSuccessOrErr = socketpair(AF_UNIX, SOCK_DGRAM, 0, PipeFiles);
+    if (PipeSuccessOrErr != 0) {
+      return make_error<Failure>(
+          "Failed to create a pipe for interprocess communication between "
+          "llvm-exegesis and the benchmarking subprocess: " +
+          Twine(strerror(errno)));
+    }
+
+    SubprocessMemory SPMemory;
+    Error MemoryInitError = SPMemory.initializeSubprocessMemory(getpid());
+    if (MemoryInitError)
+      return MemoryInitError;
+
+    Error AddMemDefError =
+        SPMemory.addMemoryDefinition(Key.MemoryValues, getpid());
+    if (AddMemDefError)
+      return AddMemDefError;
+
+    pid_t ParentOrChildPID = fork();
+
+    if (ParentOrChildPID == -1) {
+      return make_error<Failure>("Failed to create child process: " +
+                                 Twine(strerror(errno)));
+    }
+
+    if (ParentOrChildPID == 0) {
+      // We are in the child process, close the write end of the pipe.
+      close(PipeFiles[1]);
+      // Unregister handlers, signal handling is now handled through ptrace in
+      // the host process.
+      sys::unregisterHandlers();
+      runChildSubprocess(PipeFiles[0], Key);
+      // The child process terminates in the above function, so we should never
+      // get to this point.
+      llvm_unreachable("Child process didn't exit when expected.");
+    }
+
+    // Close the read end of the pipe as we only need to write to the subprocess
+    // from the parent process.
+    close(PipeFiles[0]);
+    return runParentProcess(ParentOrChildPID, PipeFiles[1], CounterName,
+                            CounterValues, ValidationCounters,
+                            ValidationCounterValues);
+  }
+
   void disableCoreDumps() const {
     struct rlimit rlim;
 
     rlim.rlim_cur = 0;
     setrlimit(RLIMIT_CORE, &rlim);
   }
 
-  [[noreturn]] void prepareAndRunBenchmark(int Pipe,
-                                           const BenchmarkKey &Key) const {
+  [[noreturn]] void runChildSubprocess(int Pipe,
+                                       const BenchmarkKey &Key) const {
     // Disable core dumps in the child process as otherwise everytime we
     // encounter an execution failure like a segmentation fault, we will create
     // a core dump. We report the information directly rather than require the
