[llvm-exegesis] Refactor ExecutableFunction to use a named constructor (#72837)

This patch refactors ExecutableFunction to use a named constructor
pattern, namely adding the create function, so that errors occurring
during the creation of an ExecutableFunction can be propogated back up
rather than having to deal with them in report_fatal_error.

Author: boomanaiden154

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

@@ -347,38 +347,44 @@ class TrackingSectionMemoryManager : public SectionMemoryManager {
 
 } // namespace
 
-ExecutableFunction::ExecutableFunction(
+Expected<ExecutableFunction> ExecutableFunction::create(
     std::unique_ptr<LLVMTargetMachine> TM,
-    object::OwningBinary<object::ObjectFile> &&ObjectFileHolder)
-    : Context(std::make_unique<LLVMContext>()) {
+    object::OwningBinary<object::ObjectFile> &&ObjectFileHolder) {
   assert(ObjectFileHolder.getBinary() && "cannot create object file");
+  std::unique_ptr<LLVMContext> Ctx = std::make_unique<LLVMContext>();
   // Initializing the execution engine.
   // We need to use the JIT EngineKind to be able to add an object file.
   LLVMLinkInMCJIT();
   uintptr_t CodeSize = 0;
   std::string Error;
-  ExecEngine.reset(
-      EngineBuilder(createModule(Context, TM->createDataLayout()))
+  std::unique_ptr<ExecutionEngine> EE(
+      EngineBuilder(createModule(Ctx, TM->createDataLayout()))
           .setErrorStr(&Error)
           .setMCPU(TM->getTargetCPU())
           .setEngineKind(EngineKind::JIT)
           .setMCJITMemoryManager(
               std::make_unique<TrackingSectionMemoryManager>(&CodeSize))
           .create(TM.release()));
-  if (!ExecEngine)
-    report_fatal_error(Twine(Error));
+  if (!EE)
+    return make_error<StringError>(Twine(Error), inconvertibleErrorCode());
   // Adding the generated object file containing the assembled function.
   // The ExecutionEngine makes sure the object file is copied into an
   // executable page.
-  ExecEngine->addObjectFile(std::move(ObjectFileHolder));
+  EE->addObjectFile(std::move(ObjectFileHolder));
   // Fetching function bytes.
-  const uint64_t FunctionAddress = ExecEngine->getFunctionAddress(FunctionID);
+  const uint64_t FunctionAddress = EE->getFunctionAddress(FunctionID);
   assert(isAligned(kFunctionAlignment, FunctionAddress) &&
          "function is not properly aligned");
-  FunctionBytes =
+  StringRef FBytes =
       StringRef(reinterpret_cast<const char *>(FunctionAddress), CodeSize);
+  return ExecutableFunction(std::move(Ctx), std::move(EE), FBytes);
 }
 
+ExecutableFunction::ExecutableFunction(std::unique_ptr<LLVMContext> Ctx,
+                                       std::unique_ptr<ExecutionEngine> EE,
+                                       StringRef FB)
+    : FunctionBytes(FB), Context(std::move(Ctx)), ExecEngine(std::move(EE)) {}
+
 Error getBenchmarkFunctionBytes(const StringRef InputData,
                                 std::vector<uint8_t> &Bytes) {
   const auto Holder = getObjectFromBuffer(InputData);

llvm/tools/llvm-exegesis/lib/Assembler.h

@@ -106,10 +106,11 @@ object::OwningBinary<object::ObjectFile> getObjectFromFile(StringRef Filename);
 
 // Consumes an ObjectFile containing a `void foo(char*)` function and make it
 // executable.
-struct ExecutableFunction {
-  explicit ExecutableFunction(
-      std::unique_ptr<LLVMTargetMachine> TM,
-      object::OwningBinary<object::ObjectFile> &&ObjectFileHolder);
+class ExecutableFunction {
+public:
+  static Expected<ExecutableFunction>
+  create(std::unique_ptr<LLVMTargetMachine> TM,
+         object::OwningBinary<object::ObjectFile> &&ObjectFileHolder);
 
   // Retrieves the function as an array of bytes.
   StringRef getFunctionBytes() const { return FunctionBytes; }
@@ -119,9 +120,15 @@ struct ExecutableFunction {
     ((void (*)(char *))(intptr_t)FunctionBytes.data())(Memory);
   }
 
+  StringRef FunctionBytes;
+
+private:
+  ExecutableFunction(std::unique_ptr<LLVMContext> Ctx,
+                     std::unique_ptr<ExecutionEngine> EE,
+                     StringRef FunctionBytes);
+
   std::unique_ptr<LLVMContext> Context;
   std::unique_ptr<ExecutionEngine> ExecEngine;
-  StringRef FunctionBytes;
 };
 
 // Copies benchmark function's bytes from benchmark object.

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

@@ -89,13 +89,25 @@ BenchmarkRunner::FunctionExecutor::runAndSample(const char *Counters) const {
 namespace {
 class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
 public:
+  static Expected<std::unique_ptr<InProcessFunctionExecutorImpl>>
+  create(const LLVMState &State, object::OwningBinary<object::ObjectFile> Obj,
+         BenchmarkRunner::ScratchSpace *Scratch) {
+    Expected<ExecutableFunction> EF =
+        ExecutableFunction::create(State.createTargetMachine(), std::move(Obj));
+
+    if (!EF)
+      return EF.takeError();
+
+    return std::unique_ptr<InProcessFunctionExecutorImpl>(
+        new InProcessFunctionExecutorImpl(State, std::move(*EF), Scratch));
+  }
+
+private:
   InProcessFunctionExecutorImpl(const LLVMState &State,
-                                object::OwningBinary<object::ObjectFile> Obj,
+                                ExecutableFunction Function,
                                 BenchmarkRunner::ScratchSpace *Scratch)
-      : State(State), Function(State.createTargetMachine(), std::move(Obj)),
-        Scratch(Scratch) {}
+      : State(State), Function(std::move(Function)), Scratch(Scratch) {}
 
-private:
   static void
   accumulateCounterValues(const llvm::SmallVector<int64_t, 4> &NewValues,
                           llvm::SmallVector<int64_t, 4> *Result) {
@@ -161,13 +173,24 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
 class SubProcessFunctionExecutorImpl
     : public BenchmarkRunner::FunctionExecutor {
 public:
+  static Expected<std::unique_ptr<SubProcessFunctionExecutorImpl>>
+  create(const LLVMState &State, object::OwningBinary<object::ObjectFile> Obj,
+         const BenchmarkKey &Key) {
+    Expected<ExecutableFunction> EF =
+        ExecutableFunction::create(State.createTargetMachine(), std::move(Obj));
+    if (!EF)
+      return EF.takeError();
+
+    return std::unique_ptr<SubProcessFunctionExecutorImpl>(
+        new SubProcessFunctionExecutorImpl(State, std::move(*EF), Key));
+  }
+
+private:
   SubProcessFunctionExecutorImpl(const LLVMState &State,
-                                 object::OwningBinary<object::ObjectFile> Obj,
+                                 ExecutableFunction Function,
                                  const BenchmarkKey &Key)
-      : State(State), Function(State.createTargetMachine(), std::move(Obj)),
-        Key(Key) {}
+      : State(State), Function(std::move(Function)), Key(Key) {}
 
-private:
   enum ChildProcessExitCodeE {
     CounterFDReadFailed = 1,
     RSeqDisableFailed,
@@ -497,17 +520,27 @@ BenchmarkRunner::createFunctionExecutor(
     object::OwningBinary<object::ObjectFile> ObjectFile,
     const BenchmarkKey &Key) const {
   switch (ExecutionMode) {
-  case ExecutionModeE::InProcess:
-    return std::make_unique<InProcessFunctionExecutorImpl>(
+  case ExecutionModeE::InProcess: {
+    auto InProcessExecutorOrErr = InProcessFunctionExecutorImpl::create(
         State, std::move(ObjectFile), Scratch.get());
-  case ExecutionModeE::SubProcess:
+    if (!InProcessExecutorOrErr)
+      return InProcessExecutorOrErr.takeError();
+
+    return std::move(*InProcessExecutorOrErr);
+  }
+  case ExecutionModeE::SubProcess: {
 #ifdef __linux__
-    return std::make_unique<SubProcessFunctionExecutorImpl>(
+    auto SubProcessExecutorOrErr = SubProcessFunctionExecutorImpl::create(
         State, std::move(ObjectFile), Key);
+    if (!SubProcessExecutorOrErr)
+      return SubProcessExecutorOrErr.takeError();
+
+    return std::move(*SubProcessExecutorOrErr);
 #else
     return make_error<Failure>(
         "The subprocess execution mode is only supported on Linux");
 #endif
+  }
   }
   llvm_unreachable("ExecutionMode is outside expected range");
 }

llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h

@@ -20,6 +20,7 @@
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/TargetParser/Host.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -82,8 +83,13 @@ class MachineFunctionGeneratorBaseTest : public ::testing::Test {
     EXPECT_FALSE(assembleToStream(*ET, createTargetMachine(), /*LiveIns=*/{},
                                   RegisterInitialValues, Fill, AsmStream, Key,
                                   false));
-    return ExecutableFunction(createTargetMachine(),
-                              getObjectFromBuffer(AsmStream.str()));
+    Expected<ExecutableFunction> ExecFunc = ExecutableFunction::create(
+        createTargetMachine(), getObjectFromBuffer(AsmStream.str()));
+
+    // We can't use ASSERT_THAT_EXPECTED here as it doesn't work inside of
+    // non-void functions.
+    EXPECT_TRUE(detail::TakeExpected(ExecFunc).Success());
+    return std::move(*ExecFunc);
   }
 
   const std::string TT;