[llvm-exegesis] Support older kernel versions in subprocess executor

[aidengrossman]

Summary

This patch switches from moving the performance counter file descriptor to the child process to socket calls rather than using the pidfd_getfd system call which was introduced in kernel 5.6. This significantly expands the range of kernel versions that are supported.

[aidengrossman]

Remove unnecessary error message, fix function name.

[aidengrossman]

Ping. This is ready for review when the reviewers have a chance. Thanks!

[courbet]

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp

205

I think this is technically UB in C++ (it violates strict aliasing). I'd be more comfortable with memcpy(CMSG_DATA(ControlMessage)), FD, sizeof(FD));, which does the same thing while having defined semantics.

232

I'm not sure you cant just read 4 bytes from the message unconditionally, you need to check that they are indeed available by checking cmsg_len.

struct cmsghdr {
    socklen_t cmsg_len;    /* data byte count, including header */
   ...
};

233

Ditto:

int FD;
mempcy(&FD, RawData, sizeof(FD));

[MaskRay]

Reminder that please make sure a reviewed commit has Differential Revision:. If you use arc diff 'HEAD^' to upload a differential, you'll get this automatically.
I typically run arc amend before pushing to get Reviewed By:

[aidengrossman]

@MaskRay Did you mean to post that on another patch? This one hasn't landed yet. There are also a couple minor patches that I committed without review that added preprocessor directives/very minor fixes related to kernel versioning to fix some build configurations that broke.

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

@courbet This should be ready for another round of review when you have time. Thanks!

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp

205

Thanks for the suggestion! I definitely wasn't particularly happy with that level of casting, not sure why a memcpy didn't come up to me. That is definitely a lot cleaner and avoids the UB (and compiler warning).

[uabelho]

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp

224

Should be ssize_t ?

The

BytesRead < 0

check below is currently always false since size_t is unsigned.

I noticed this when I compiled with gcc which warned

../tools/llvm-exegesis/lib/BenchmarkRunner.cpp:226:19: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
  226 |     if (BytesRead < 0)
      |         ~~~~~~~~~~^~~

[aidengrossman]

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp

224

Yep. Thanks for pointing this out! Not sure how I missed it. recvmsg definitely returns ssize_t.

Fixed in 34e3bc0b92d7fdc59477c93d187546a2566fbc22.

llvm/test/tools/llvm-exegesis/lit.local.cfg

try:
             stderr=subprocess.DEVNULL,
         )
         return return_code == 0
     except OSError:
         print("could not exec llvm-exegesis")
         return False
 
 
-def can_execute_in_subprocess():
-    # We need certain Linux system calls present in order to run the subprocess
-    # executor mode, so check that we can use the subprocess mode to prevent
-    # test failures on platforms running older kernels.
-    llvm_exegesis_exe = lit.util.which("llvm-exegesis", config.llvm_tools_dir)
-    if llvm_exegesis_exe is None:
-        print("could not find llvm-exegesis")
-        return False
-    try:
-        command_vector = [
-            llvm_exegesis_exe,
-            "-mode=latency",
-            "--execution-mode=subprocess",
-            "-snippets-file=/dev/null",
-        ]
-        with subprocess.Popen(
-            command_vector, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL
-        ) as exegesis_subprocess:
-            stdout, stderr = exegesis_subprocess.communicate()
-            exegesis_output = stdout.decode("utf-8")
-            # Return true if we have an empty error section as llvm-exegesis
-            # doesn't change the return code if there's a snippet crash.
-            return re.search("error:\s*''", exegesis_output) is not None
-
-    except OSError:
-        print("Could not execute llvm-exegesis in subprocess executor mode")
-        return False
-
-
 for arch in ["aarch64", "mips", "powerpc", "x86_64"]:
     if can_execute_generated_snippets(arch):
         config.available_features.add("exegesis-can-execute-%s" % arch)
 
 if can_use_perf_counters("latency"):
     config.available_features.add("exegesis-can-measure-latency")
 
 if can_use_perf_counters("uops"):
     config.available_features.add("exegesis-can-measure-uops")
 
 if can_execute_generated_snippets("x86_64"):
     # Check for support of LBR format with cycles.
     if can_use_perf_counters(
         "latency", ["-x86-lbr-sample-period", "123", "-repetition-mode", "loop"]
     ):
         config.available_features.add("exegesis-can-measure-latency-lbr")
 
 # The subprocess tests are flaky on some of the builders that support them, so
 # they are disabled currently.
-#if can_execute_in_subprocess():
+#if True:
 #    config.available_features.add("exegesis-can-execute-in-subprocess")
+

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp

 #include "llvm/Support/Signals.h"
 
 #ifdef __linux__
 #ifdef HAVE_LIBPFM
 #include <perfmon/perf_event.h>
 #endif
 #include <sys/mman.h>
 #include <sys/ptrace.h>
+#include <sys/socket.h>
 #include <sys/syscall.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #if defined(__GLIBC__) && __has_include(<sys/rseq.h>) && __has_builtin(__builtin_thread_pointer)
 #include <sys/rseq.h>
 #ifdef RSEQ_SIG
 #define GLIBC_INITS_RSEQ
SubProcessFunctionExecutorImpl(const LLVMState &State,
                                  object::OwningBinary<object::ObjectFile> Obj,
                                  const BenchmarkKey &Key)
       : State(State), Function(State.createTargetMachine(), std::move(Obj)),
         Key(Key) {}
 
 private:
   enum ChildProcessExitCodeE {
     CounterFDReadFailed = 1,
-    TranslatingCounterFDFailed,
     RSeqDisableFailed,
     FunctionDataMappingFailed,
     AuxiliaryMemorySetupFailed
-
   };
 
   StringRef childProcessExitCodeToString(int ExitCode) const {
     switch (ExitCode) {
     case ChildProcessExitCodeE::CounterFDReadFailed:
       return "Counter file descriptor read failed";
-    case ChildProcessExitCodeE::TranslatingCounterFDFailed:
-      return "Translating counter file descriptor into a file descriptor in "
-             "the child process failed. This might be due running an older "
-             "Linux kernel that doesn't support the pidfd_getfd system call "
-             "(anything before Linux 5.6).";
     case ChildProcessExitCodeE::RSeqDisableFailed:
       return "Disabling restartable sequences failed";
     case ChildProcessExitCodeE::FunctionDataMappingFailed:
       return "Failed to map memory for assembled snippet";
     case ChildProcessExitCodeE::AuxiliaryMemorySetupFailed:
       return "Failed to setup auxiliary memory";
     default:
       return "Child process returned with unknown exit code";
     }
   }
 
+  Error sendFileDescriptorThroughSocket(int SocketFD, int FD) const {
+    struct msghdr Message = {};
+    char Buffer[CMSG_SPACE(sizeof(FD))];
+    memset(Buffer, 0, sizeof(Buffer));
+    Message.msg_control = Buffer;
+    Message.msg_controllen = sizeof(Buffer);
+
+    struct cmsghdr *ControlMessage = CMSG_FIRSTHDR(&Message);
+    ControlMessage->cmsg_level = SOL_SOCKET;
+    ControlMessage->cmsg_type = SCM_RIGHTS;
+    ControlMessage->cmsg_len = CMSG_LEN(sizeof(FD));
+
+    memcpy(CMSG_DATA(ControlMessage), &FD, sizeof(FD));
+
+    Message.msg_controllen = CMSG_SPACE(sizeof(FD));
+
+    ssize_t BytesWritten = sendmsg(SocketFD, &Message, 0);
+
+    if (BytesWritten < 0)
+      return make_error<Failure>("Failed to write FD to socket");
+
+    return Error::success();
+  }
+
+  Expected<int> getFileDescriptorFromSocket(int SocketFD) const {
+    struct msghdr Message = {};
+
+    char ControlBuffer[256];
+    Message.msg_control = ControlBuffer;
+    Message.msg_controllen = sizeof(ControlBuffer);
+
+    size_t BytesRead = recvmsg(SocketFD, &Message, 0);
+
+    if (BytesRead < 0)
+      return make_error<Failure>("Failed to read FD from socket");
+
+    struct cmsghdr *ControlMessage = CMSG_FIRSTHDR(&Message);
+
+    int FD;
+
+    if (ControlMessage->cmsg_len != CMSG_LEN(sizeof(FD)))
+      return make_error<Failure>("Failed to get correct number of bytes for "
+                                 "file descriptor from socket.");
+
+    memcpy(&FD, CMSG_DATA(ControlMessage), sizeof(FD));
+
+    return FD;
+  }
+
   Error createSubProcessAndRunBenchmark(
       StringRef CounterName, SmallVectorImpl<int64_t> &CounterValues) const {
     int PipeFiles[2];
-    int PipeSuccessOrErr = pipe(PipeFiles);
+    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");
     }
 
     SubprocessMemory SPMemory;
     Error MemoryInitError = SPMemory.initializeSubprocessMemory(getpid());
Error createSubProcessAndRunBenchmark(
     if (!CounterOrError)
       return CounterOrError.takeError();
 
     pfm::Counter *Counter = CounterOrError.get().get();
 
     close(PipeFiles[0]);
 
     int CounterFileDescriptor = Counter->getFileDescriptor();
-    ssize_t BytesWritten =
-        write(PipeFiles[1], &CounterFileDescriptor, sizeof(int));
+    Error SendError =
+        sendFileDescriptorThroughSocket(PipeFiles[1], CounterFileDescriptor);
 
-    if (BytesWritten != sizeof(int))
-      return make_error<Failure>("Writing peformance counter file descriptor "
-                                 "to child process failed: " +
-                                 Twine(strerror(errno)));
+    if (SendError)
+      return SendError;
 
     if (ptrace(PTRACE_ATTACH, ParentOrChildPID, NULL, NULL) != 0)
       return make_error<Failure>("Failed to attach to the child process: " +
                                  Twine(strerror(errno)));
 
     if (wait(NULL) == -1) {
       return make_error<Failure>(
           "Failed to wait for child process to stop after attaching: " +
Error createSubProcessAndRunBenchmark(
     return make_error<SnippetCrash>(
         "The benchmarking subprocess sent unexpected signal: " +
         Twine(strsignal(ChildSignalInfo.si_signo)));
   }
 
   [[noreturn]] void prepareAndRunBenchmark(int Pipe,
                                            const BenchmarkKey &Key) const {
     // The following occurs within the benchmarking subprocess
+    pid_t ParentPID = getppid();
 
-    int ParentCounterFileDescriptor = -1;
-    ssize_t BytesRead = read(Pipe, &ParentCounterFileDescriptor, sizeof(int));
+    Expected<int> CounterFileDescriptorOrError =
+        getFileDescriptorFromSocket(Pipe);
 
-    if (BytesRead != sizeof(int)) {
+    if (!CounterFileDescriptorOrError)
       exit(ChildProcessExitCodeE::CounterFDReadFailed);
-    }
-
-    pid_t ParentPID = getppid();
 
-    // Make sure the following two syscalls are defined on the platform that
-    // we're building on as they were introduced to the kernel fairly recently
-    // (v5.6 for the second one).
-#if defined(SYS_pidfd_open) && defined(SYS_pidfd_getfd)
-    int ParentPIDFD = syscall(SYS_pidfd_open, ParentPID, 0);
-    int CounterFileDescriptor =
-        syscall(SYS_pidfd_getfd, ParentPIDFD, ParentCounterFileDescriptor, 0);
-#else
-    int CounterFileDescriptor = 0;
-    exit(ChildProcessExitCodeE::TranslatingCounterFDFailed);
-#endif
-
-    if (CounterFileDescriptor == -1) {
-      exit(ChildProcessExitCodeE::TranslatingCounterFDFailed);
-    }
+    int CounterFileDescriptor = *CounterFileDescriptorOrError;
 
 // Glibc versions greater than 2.35 automatically call rseq during
 // initialization. Unmapping the region that glibc sets up for this causes
 // segfaults in the program Unregister the rseq region so that we can safely
 // unmap it later
 #ifdef GLIBC_INITS_RSEQ
     long RseqDisableOutput =
         syscall(SYS_rseq, (intptr_t)__builtin_thread_pointer() + __rseq_offset,