[orc-rt] Hoist run-alloc-actions functions, add error reporting. (#209356)

Hoist runFinalizeActions and runDeallocActions into AllocAction.h and
turn them into function templates with a ReportErrorFn argument that
enables error reporting. This is used to report errors from dealloc
actions.

Update SimpleNativeMemoryMap to direct such errors to the Session via
ReportErrorsViaSession.

Add unit tests covering the new error-reporting paths.

orc-rt/include/orc-rt/AllocAction.h

@@ -16,6 +16,7 @@
 #include "orc-rt/CallableTraitsHelper.h"
 #include "orc-rt/Error.h"
 #include "orc-rt/WrapperFunction.h"
+#include "orc-rt/scope_exit.h"
 
 #include <vector>
 
@@ -74,11 +75,45 @@ struct AllocActionPair {
 ///
 /// Both finalize and dealloc actions are permitted to be null (i.e. have a
 /// null action function) in which case they are ignored.
+template <typename ReportErrorFn>
 Expected<std::vector<AllocAction>>
-runFinalizeActions(std::vector<AllocActionPair> AAPs);
+runFinalizeActions(std::vector<AllocActionPair> AAPs,
+                   ReportErrorFn &&ReportError) {
+  std::vector<AllocAction> DeallocActions;
+  auto RunDeallocActions = scope_exit([&]() {
+    while (!DeallocActions.empty()) {
+      auto B = DeallocActions.back()();
+      if (auto *ErrMsg = B.getOutOfBandError())
+        ReportError(make_error<StringError>(ErrMsg));
+      DeallocActions.pop_back();
+    }
+  });
+
+  for (auto &AAP : AAPs) {
+    if (AAP.Finalize) {
+      auto B = AAP.Finalize();
+      if (const char *ErrMsg = B.getOutOfBandError())
+        return make_error<StringError>(ErrMsg);
+    }
+    if (AAP.Dealloc)
+      DeallocActions.push_back(std::move(AAP.Dealloc));
+  }
+
+  RunDeallocActions.release();
+  return DeallocActions;
+}
 
 /// Run the given deallocation actions in revwerse order.
-void runDeallocActions(std::vector<AllocAction> DAAs);
+template <typename ReportErrorFn>
+void runDeallocActions(std::vector<AllocAction> DAAs,
+                       ReportErrorFn &&ReportError) {
+  while (!DAAs.empty()) {
+    auto B = DAAs.back()();
+    if (const char *ErrMsg = B.getOutOfBandError())
+      ReportError(make_error<StringError>(ErrMsg));
+    DAAs.pop_back();
+  }
+}
 
 } // namespace orc_rt
 

orc-rt/lib/executor/AllocAction.cpp

@@ -1,55 +0,0 @@
-//===- AllocAction.cpp ----------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-//
-// AllocAction and related APIs.
-//
-//===----------------------------------------------------------------------===//
-
-#include "orc-rt/AllocAction.h"
-#include "orc-rt/scope_exit.h"
-
-namespace orc_rt {
-
-Expected<std::vector<AllocAction>>
-runFinalizeActions(std::vector<AllocActionPair> AAPs) {
-  std::vector<AllocAction> DeallocActions;
-  auto RunDeallocActions = scope_exit([&]() {
-    while (!DeallocActions.empty()) {
-      // TODO: Log errors from cleanup dealloc actions.
-      {
-        [[maybe_unused]] auto B = DeallocActions.back()();
-      }
-      DeallocActions.pop_back();
-    }
-  });
-
-  for (auto &AAP : AAPs) {
-    if (AAP.Finalize) {
-      auto B = AAP.Finalize();
-      if (const char *ErrMsg = B.getOutOfBandError())
-        return make_error<StringError>(ErrMsg);
-    }
-    if (AAP.Dealloc)
-      DeallocActions.push_back(std::move(AAP.Dealloc));
-  }
-
-  RunDeallocActions.release();
-  return DeallocActions;
-}
-
-void runDeallocActions(std::vector<AllocAction> DAAs) {
-  while (!DAAs.empty()) {
-    // TODO: Log errors from cleanup dealloc actions.
-    {
-      [[maybe_unused]] auto B = DAAs.back()();
-    }
-    DAAs.pop_back();
-  }
-}
-
-} // namespace orc_rt

orc-rt/lib/executor/CMakeLists.txt

@@ -1,5 +1,4 @@
 set(files
-  AllocAction.cpp
   BootstrapInfo.cpp
   SimpleSymbolTable.cpp
   Environment.cpp

orc-rt/lib/executor/SimpleNativeMemoryMap.cpp

@@ -93,7 +93,7 @@ void SimpleNativeMemoryMap::release(OnReleaseCompleteFn &&OnComplete,
   }
 
   for (auto &[Addr, DAAs] : SI->DeallocActions)
-    runDeallocActions(std::move(DAAs));
+    runDeallocActions(std::move(DAAs), ReportErrorsViaSession(S));
 
   OnComplete(hostOSMemoryRelease(Addr, SI->Size));
 }
@@ -153,12 +153,13 @@ void SimpleNativeMemoryMap::initialize(OnInitializeCompleteFn &&OnComplete,
                                 "finalization requires at least "
                                 "one standard-lifetime segment"));
 
-  auto DeallocActions = runFinalizeActions(std::move(IR.AAPs));
+  auto DeallocActions =
+      runFinalizeActions(std::move(IR.AAPs), ReportErrorsViaSession(S));
   if (!DeallocActions)
     return OnComplete(DeallocActions.takeError());
 
   if (auto Err = recordDeallocActions(Base, std::move(*DeallocActions))) {
-    runDeallocActions(std::move(*DeallocActions));
+    runDeallocActions(std::move(*DeallocActions), ReportErrorsViaSession(S));
     return OnComplete(std::move(Err));
   }
 
@@ -192,7 +193,7 @@ void SimpleNativeMemoryMap::deinitialize(OnDeinitializeCompleteFn &&OnComplete,
     SI->DeallocActions.erase(I);
   }
 
-  runDeallocActions(std::move(DAAs));
+  runDeallocActions(std::move(DAAs), ReportErrorsViaSession(S));
   OnComplete(Error::success());
 }
 

orc-rt/test/unit/AllocActionTest.cpp

@@ -18,6 +18,8 @@
 
 #include "gtest/gtest.h"
 
+#include "CommonTestUtils.h"
+
 #include <cstring>
 
 using namespace orc_rt;
@@ -84,6 +86,13 @@ static orc_rt_WrapperFunctionBuffer fail_action(const char *ArgData,
   return WrapperFunctionBuffer::createOutOfBandError("failed").release();
 }
 
+// Always returns an out-of-band error with a distinct message. Used to tell
+// finalize-path and dealloc-path failures apart in error-reporting tests.
+static orc_rt_WrapperFunctionBuffer fail_action_2(const char *ArgData,
+                                                  size_t ArgSize) {
+  return WrapperFunctionBuffer::createOutOfBandError("failed_2").release();
+}
+
 TEST(AllocActionTest, DefaultConstruct) {
   AllocAction AA;
   EXPECT_FALSE(AA);
@@ -116,29 +125,40 @@ TEST(AllocActionTest, RunFinalizationActionsComplete) {
   InitialActions.push_back({MakeAAOnVal(increment_int_ptr_action),
                             MakeAAOnVal(decrement_int_ptr_action)});
 
-  auto DeallocActions = cantFail(runFinalizeActions(std::move(InitialActions)));
+  auto DeallocActions =
+      cantFail(runFinalizeActions(std::move(InitialActions), noErrors));
 
   EXPECT_EQ(Val, 2);
 
-  runDeallocActions(std::move(DeallocActions));
+  runDeallocActions(std::move(DeallocActions), noErrors);
 
   EXPECT_EQ(Val, 0);
 }
 
 TEST(AllocActionTest, RunFinalizeActionsFail) {
-  int Val = 0;
+  int SucceedingPairVal = 0;
+  int FailingPairDeallocVal = 0;
+  int AfterFailurePairVal = 0;
 
   std::vector<AllocActionPair> InitialActions;
 
-  auto MakeAAOnVal = [&](AllocActionFn Fn) {
-    return AllocAction(Fn, makeIntPtrArgBuffer(&Val));
+  auto MakeAA = [&](AllocActionFn Fn, int *P) {
+    return AllocAction(Fn, makeIntPtrArgBuffer(P));
   };
-  InitialActions.push_back({MakeAAOnVal(increment_int_ptr_action),
-                            MakeAAOnVal(decrement_int_ptr_action)});
-  InitialActions.push_back({AllocAction(fail_action, WrapperFunctionBuffer()),
-                            MakeAAOnVal(decrement_int_ptr_action)});
+  // First pair's finalize and dealloc actions both increment SucceedingPairVal.
+  InitialActions.push_back(
+      {MakeAA(increment_int_ptr_action, &SucceedingPairVal),
+       MakeAA(increment_int_ptr_action, &SucceedingPairVal)});
+  // Second pair's finalize fails, so its dealloc action must not run.
+  InitialActions.push_back(
+      {AllocAction(fail_action, WrapperFunctionBuffer()), // finalize fails
+       MakeAA(increment_int_ptr_action, &FailingPairDeallocVal)});
+  // Third pair: finalize actions past the failure point must not run.
+  InitialActions.push_back(
+      {MakeAA(increment_int_ptr_action, &AfterFailurePairVal),
+       MakeAA(increment_int_ptr_action, &AfterFailurePairVal)});
 
-  auto DeallocActions = runFinalizeActions(std::move(InitialActions));
+  auto DeallocActions = runFinalizeActions(std::move(InitialActions), noErrors);
 
   if (DeallocActions) {
     ADD_FAILURE() << "Failed to report error from runFinalizeActions";
@@ -147,8 +167,15 @@ TEST(AllocActionTest, RunFinalizeActionsFail) {
 
   EXPECT_EQ(toString(DeallocActions.takeError()), std::string("failed"));
 
-  // Check that we ran the decrement corresponding to the first increment.
-  EXPECT_EQ(Val, 0);
+  // First pair fully ran: +1 from its finalize action, +1 from its dealloc
+  // action during cleanup.
+  EXPECT_EQ(SucceedingPairVal, 2);
+
+  // Second pair's finalize failed, so its dealloc action must not run.
+  EXPECT_EQ(FailingPairDeallocVal, 0);
+
+  // Third pair's finalize is past the failure point, so it must not run.
+  EXPECT_EQ(AfterFailurePairVal, 0);
 }
 
 TEST(AllocActionTest, RunFinalizeActionsNullFinalize) {
@@ -164,13 +191,14 @@ TEST(AllocActionTest, RunFinalizeActionsNullFinalize) {
   InitialActions.push_back({AllocAction(nullptr, WrapperFunctionBuffer()),
                             MakeAAOnVal(decrement_int_ptr_action)});
 
-  auto DeallocActions = cantFail(runFinalizeActions(std::move(InitialActions)));
+  auto DeallocActions =
+      cantFail(runFinalizeActions(std::move(InitialActions), noErrors));
 
   // Both dealloc actions should be included in the returned list, despite one
   // of them having a null finalize action.
   EXPECT_EQ(DeallocActions.size(), 2U);
 
-  runDeallocActions(std::move(DeallocActions));
+  runDeallocActions(std::move(DeallocActions), noErrors);
 
   EXPECT_EQ(Val, -1);
 }
@@ -188,12 +216,83 @@ TEST(AllocActionTest, RunFinalizeActionsNullDealloc) {
   InitialActions.push_back({MakeAAOnVal(increment_int_ptr_action),
                             AllocAction(nullptr, WrapperFunctionBuffer())});
 
-  auto DeallocActions = cantFail(runFinalizeActions(std::move(InitialActions)));
+  auto DeallocActions =
+      cantFail(runFinalizeActions(std::move(InitialActions), noErrors));
 
   // Null dealloc actions should be filtered out of the returned list.
   EXPECT_EQ(DeallocActions.size(), 1U);
 
-  runDeallocActions(std::move(DeallocActions));
+  runDeallocActions(std::move(DeallocActions), noErrors);
 
   EXPECT_EQ(Val, 1);
 }
+
+TEST(AllocActionTest, RunDeallocActionsReportsError) {
+  int Val = 0;
+
+  std::vector<AllocAction> DeallocActions;
+  // First dealloc action runs second (dealloc runs in reverse), but must still
+  // run despite the failure reported below.
+  DeallocActions.push_back(
+      AllocAction(increment_int_ptr_action, makeIntPtrArgBuffer(&Val)));
+  // Runs first: fails and should be reported.
+  DeallocActions.push_back(AllocAction(fail_action, WrapperFunctionBuffer()));
+
+  std::vector<std::string> ErrMsgs;
+  runDeallocActions(std::move(DeallocActions), AccumulateErrors(ErrMsgs));
+
+  ASSERT_EQ(ErrMsgs.size(), 1U);
+  EXPECT_EQ(ErrMsgs[0], "failed");
+
+  // The non-failing dealloc action still ran.
+  EXPECT_EQ(Val, 1);
+}
+
+TEST(AllocActionTest, RunDeallocActionsReportsAllErrors) {
+  std::vector<AllocAction> DeallocActions;
+  DeallocActions.push_back(AllocAction(fail_action, WrapperFunctionBuffer()));
+  DeallocActions.push_back(AllocAction(fail_action_2, WrapperFunctionBuffer()));
+
+  std::vector<std::string> ErrMsgs;
+  runDeallocActions(std::move(DeallocActions), AccumulateErrors(ErrMsgs));
+
+  // Both failures reported; dealloc runs in reverse, so fail_action_2 first.
+  ASSERT_EQ(ErrMsgs.size(), 2U);
+  EXPECT_EQ(ErrMsgs[0], "failed_2");
+  EXPECT_EQ(ErrMsgs[1], "failed");
+}
+
+TEST(AllocActionTest, RunFinalizeActionsFailReportsCleanupErrors) {
+  int Val = 0;
+
+  std::vector<AllocActionPair> InitialActions;
+  // Finalize succeeds; its dealloc decrements Val during cleanup. Proves
+  // cleanup continues past the failure of the pair below.
+  InitialActions.push_back(
+      {AllocAction(increment_int_ptr_action, makeIntPtrArgBuffer(&Val)),
+       AllocAction(decrement_int_ptr_action, makeIntPtrArgBuffer(&Val))});
+  // Finalize succeeds; its dealloc fails during the cleanup triggered below.
+  InitialActions.push_back(
+      {AllocAction(increment_int_ptr_action, makeIntPtrArgBuffer(&Val)),
+       AllocAction(fail_action, WrapperFunctionBuffer())});
+  // Finalize fails, triggering cleanup of the accumulated dealloc actions.
+  InitialActions.push_back({AllocAction(fail_action_2, WrapperFunctionBuffer()),
+                            AllocAction(nullptr, WrapperFunctionBuffer())});
+
+  std::vector<std::string> ErrMsgs;
+  auto DeallocActions =
+      runFinalizeActions(std::move(InitialActions), AccumulateErrors(ErrMsgs));
+
+  // The finalize failure is the returned error...
+  ASSERT_FALSE(DeallocActions);
+  EXPECT_EQ(toString(DeallocActions.takeError()), std::string("failed_2"));
+
+  // ...while the cleanup dealloc failure is reported out-of-band.
+  ASSERT_EQ(ErrMsgs.size(), 1U);
+  EXPECT_EQ(ErrMsgs[0], "failed");
+
+  // Both finalize actions ran (Val += 2) and both cleanup dealloc actions ran:
+  // the failing one left Val untouched, the surviving one decremented it. That
+  // the decrement took effect proves cleanup continued past the failure.
+  EXPECT_EQ(Val, 1);
+}

orc-rt/test/unit/CommonTestUtils.h

@@ -21,9 +21,25 @@
 #include <cstddef>
 #include <cstdint>
 #include <future>
+#include <string>
+#include <vector>
 
 inline void noErrors(orc_rt::Error Err) { orc_rt::cantFail(std::move(Err)); }
 
+/// ReportError callback for tests that records the message of every reported
+/// error, in the order reported.
+class AccumulateErrors {
+public:
+  AccumulateErrors(std::vector<std::string> &ErrMsgs) : ErrMsgs(ErrMsgs) {}
+
+  void operator()(orc_rt::Error Err) {
+    ErrMsgs.push_back(orc_rt::toString(std::move(Err)));
+  }
+
+private:
+  std::vector<std::string> &ErrMsgs;
+};
+
 inline orc_rt::ExecutorProcessInfo mockExecutorProcessInfo() noexcept {
   return orc_rt::ExecutorProcessInfo("arm64-apple-darwin", 16384);
 }