[llvm-exegesis] Refactor parent code to separate function

[boomanaiden154]

This patch refactors the parent code to a separate function in the subprocess executor to make the code more clear and easy to follow.

[llvmbot]

@llvm/pr-subscribers-tools-llvm-exegesis

Author: Aiden Grossman (boomanaiden154)

Changes

This patch refactors the parent code to a separate function in the subprocess executor to make the code more clear and easy to follow.

---

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

1 Files Affected:

• (modified) llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp (+63-53)

diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
index 5c9848f3c68885..df6774cd138584 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -278,59 +278,20 @@ 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
@@ -338,7 +299,7 @@ class SubProcessFunctionExecutorImpl
     // 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)));
 
@@ -348,14 +309,14 @@ class SubProcessFunctionExecutorImpl
           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;
@@ -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)));
     }
@@ -408,6 +368,56 @@ 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;
 
@@ -415,8 +425,8 @@ class SubProcessFunctionExecutorImpl
     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

[legrosbuffle]

Perfect, thanks.