[Offload] Ensure all `llvm::Error`s are handled #137339

RossBrunton · 2025-04-25T14:26:08Z

llvm::Errors containing errors must be explicitly handled or an assert
will be raised. With this change, ol_impl_result_t can accept and
consume an llvm::Error for errors raised by PluginInterface that have
multiple causes and other places now call llvm::consumeError.

Note that there is currently no facility for PluginInterface to
communicate exact error codes, but the constructor is designed in
such a way that it can be easily added later. This MR is to
convert a crash into an error code.

A new test was added, however due to the aforementioned issue with
error codes, it does not pass and instead is marked as a skip.

github-actions · 2025-04-25T14:26:27Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-04-25T14:27:01Z

@llvm/pr-subscribers-offload

Author: Ross Brunton (RossBrunton)

Changes

llvm::Errors containing errors must be explicitly handled or an assert
will be raised. With this change, ol_impl_result_t can accept and
consume an llvm::Error for errors raised by PluginInterface that have
multiple causes and other places now call llvm::consumeError.

Note that there is currently no facility for PluginInterface to
communicate exact error codes, but the constructor is designed in
such a way that it can be easily added later. This MR is to
convert a crash into an error code.

A new test was added, however due to the aforementioned issue with
error codes, it does not pass and instead is marked as a skip.

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

3 Files Affected:

(modified) offload/liboffload/include/OffloadImpl.hpp (+12)
(modified) offload/liboffload/src/OffloadImpl.cpp (+26-13)
(modified) offload/unittests/OffloadAPI/kernel/olGetKernel.cpp (+8)

diff --git a/offload/liboffload/include/OffloadImpl.hpp b/offload/liboffload/include/OffloadImpl.hpp
index ec470a355309a..570fddfa87f02 100644
--- a/offload/liboffload/include/OffloadImpl.hpp
+++ b/offload/liboffload/include/OffloadImpl.hpp
@@ -19,6 +19,7 @@
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Error.h"
 
 struct OffloadConfig {
   bool TracingEnabled = false;
@@ -88,6 +89,17 @@ struct ol_impl_result_t {
     Result = errors().emplace(std::move(Err)).first->get();
   }
 
+  ol_impl_result_t(llvm::Error &&Error, llvm::StringRef Details) {
+    ol_errc_t ErrCode;
+    llvm::handleAllErrors(std::move(Error), [&](llvm::StringError &Err) {
+      // TODO: PluginInterface doesn't yet have a way to communicate offload
+      // error codes
+      ErrCode = OL_ERRC_UNKNOWN;
+    });
+
+    ol_impl_result_t(ErrCode, Details);
+  }
+
   operator ol_result_t() { return Result; }
 
 private:
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index d956d274b5eb1..fc74771abbbdb 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -308,9 +308,11 @@ ol_impl_result_t olMemAlloc_impl(ol_device_handle_t Device,
                                  void **AllocationOut) {
   auto Alloc =
       Device->Device->dataAlloc(Size, nullptr, convertOlToPluginAllocTy(Type));
-  if (!Alloc)
+  if (!Alloc) {
+    llvm::consumeError(Alloc.takeError());
     return {OL_ERRC_OUT_OF_RESOURCES,
             formatv("Could not create allocation on device {0}", Device).str()};
+  }
 
   *AllocationOut = *Alloc;
   allocInfoMap().insert_or_assign(*Alloc, AllocInfo{Device, Type});
@@ -327,8 +329,10 @@ ol_impl_result_t olMemFree_impl(void *Address) {
 
   auto Res =
       Device->Device->dataDelete(Address, convertOlToPluginAllocTy(Type));
-  if (Res)
+  if (Res) {
+    llvm::consumeError(std::move(Res));
     return {OL_ERRC_OUT_OF_RESOURCES, "Could not free allocation"};
+  }
 
   allocInfoMap().erase(Address);
 
@@ -340,7 +344,7 @@ ol_impl_result_t olCreateQueue_impl(ol_device_handle_t Device,
   auto CreatedQueue = std::make_unique<ol_queue_impl_t>(nullptr, Device);
   auto Err = Device->Device->initAsyncInfo(&(CreatedQueue->AsyncInfo));
   if (Err)
-    return {OL_ERRC_UNKNOWN, "Could not initialize stream resource"};
+    return {std::move(Err), "Could not initialize stream resource"};
 
   *Queue = CreatedQueue.release();
   return OL_SUCCESS;
@@ -355,8 +359,10 @@ ol_impl_result_t olWaitQueue_impl(ol_queue_handle_t Queue) {
   // on it, but we have nothing to synchronize in that situation anyway.
   if (Queue->AsyncInfo->Queue) {
     auto Err = Queue->Device->Device->synchronize(Queue->AsyncInfo);
-    if (Err)
+    if (Err) {
+      llvm::consumeError(std::move(Err));
       return {OL_ERRC_INVALID_QUEUE, "The queue failed to synchronize"};
+    }
   }
 
   // Recreate the stream resource so the queue can be reused
@@ -364,15 +370,17 @@ ol_impl_result_t olWaitQueue_impl(ol_queue_handle_t Queue) {
   // it to begin with.
   auto Res = Queue->Device->Device->initAsyncInfo(&Queue->AsyncInfo);
   if (Res)
-    return {OL_ERRC_UNKNOWN, "Could not reinitialize the stream resource"};
+    return {std::move(Res), "Could not reinitialize the stream resource"};
 
   return OL_SUCCESS;
 }
 
 ol_impl_result_t olWaitEvent_impl(ol_event_handle_t Event) {
   auto Res = Event->Queue->Device->Device->syncEvent(Event->EventInfo);
-  if (Res)
+  if (Res) {
+    llvm::consumeError(std::move(Res));
     return {OL_ERRC_INVALID_EVENT, "The event failed to synchronize"};
+  }
 
   return OL_SUCCESS;
 }
@@ -384,13 +392,17 @@ ol_impl_result_t olDestroyEvent_impl(ol_event_handle_t Event) {
 ol_event_handle_t makeEvent(ol_queue_handle_t Queue) {
   auto EventImpl = std::make_unique<ol_event_impl_t>(nullptr, Queue);
   auto Res = Queue->Device->Device->createEvent(&EventImpl->EventInfo);
-  if (Res)
+  if (Res) {
+    llvm::consumeError(std::move(Res));
     return nullptr;
+  }
 
   Res = Queue->Device->Device->recordEvent(EventImpl->EventInfo,
                                            Queue->AsyncInfo);
-  if (Res)
+  if (Res) {
+    llvm::consumeError(std::move(Res));
     return nullptr;
+  }
 
   return EventImpl.release();
 }
@@ -416,16 +428,16 @@ ol_impl_result_t olMemcpy_impl(ol_queue_handle_t Queue, void *DstPtr,
   if (DstDevice == HostDevice()) {
     auto Res = SrcDevice->Device->dataRetrieve(DstPtr, SrcPtr, Size, QueueImpl);
     if (Res)
-      return {OL_ERRC_UNKNOWN, "The data retrieve operation failed"};
+      return {std::move(Res), "The data retrieve operation failed"};
   } else if (SrcDevice == HostDevice()) {
     auto Res = DstDevice->Device->dataSubmit(DstPtr, SrcPtr, Size, QueueImpl);
     if (Res)
-      return {OL_ERRC_UNKNOWN, "The data submit operation failed"};
+      return {std::move(Res), "The data submit operation failed"};
   } else {
     auto Res = SrcDevice->Device->dataExchange(SrcPtr, *DstDevice->Device,
                                                DstPtr, Size, QueueImpl);
     if (Res)
-      return {OL_ERRC_UNKNOWN, "The data exchange operation failed"};
+      return {std::move(Res), "The data exchange operation failed"};
   }
 
   if (EventOut)
@@ -452,6 +464,7 @@ ol_impl_result_t olCreateProgram_impl(ol_device_handle_t Device,
   auto Res =
       Device->Device->loadBinary(Device->Device->Plugin, &Prog->DeviceImage);
   if (!Res) {
+    llvm::consumeError(Res.takeError());
     delete Prog;
     return OL_ERRC_INVALID_VALUE;
   }
@@ -477,7 +490,7 @@ ol_impl_result_t olGetKernel_impl(ol_program_handle_t Program,
 
   auto Err = KernelImpl->init(Device, *Program->Image);
   if (Err)
-    return {OL_ERRC_UNKNOWN, "Could not initialize the kernel"};
+    return {std::move(Err), "Could not initialize the kernel"};
 
   *Kernel = &*KernelImpl;
 
@@ -520,7 +533,7 @@ olLaunchKernel_impl(ol_queue_handle_t Queue, ol_device_handle_t Device,
 
   AsyncInfoWrapper.finalize(Err);
   if (Err)
-    return {OL_ERRC_UNKNOWN, "Could not finalize the AsyncInfoWrapper"};
+    return {std::move(Err), "Could not finalize the AsyncInfoWrapper"};
 
   if (EventOut)
     *EventOut = makeEvent(Queue);
diff --git a/offload/unittests/OffloadAPI/kernel/olGetKernel.cpp b/offload/unittests/OffloadAPI/kernel/olGetKernel.cpp
index f320d191ad58f..0dbedbdebd2fd 100644
--- a/offload/unittests/OffloadAPI/kernel/olGetKernel.cpp
+++ b/offload/unittests/OffloadAPI/kernel/olGetKernel.cpp
@@ -28,3 +28,11 @@ TEST_F(olGetKernelTest, InvalidNullKernelPointer) {
   ASSERT_ERROR(OL_ERRC_INVALID_NULL_POINTER,
                olGetKernel(Program, "foo", nullptr));
 }
+
+TEST_F(olGetKernelTest, InvalidKernelName) {
+  GTEST_SKIP()
+      << "Error code returning from plugin interface not yet supported";
+  ol_kernel_handle_t Kernel = nullptr;
+  ASSERT_ERROR(OL_ERRC_INVALID_KERNEL_NAME,
+               olGetKernel(Program, "invalid_kernel_name", &Kernel));
+}

RossBrunton · 2025-04-25T14:28:32Z

offload/liboffload/include/OffloadImpl.hpp

@@ -88,6 +89,17 @@ struct ol_impl_result_t {
    Result = errors().emplace(std::move(Err)).first->get();
  }

+  ol_impl_result_t(llvm::Error &&Error, llvm::StringRef Details) {


I'm not sure if this is better as a constructor or an explicit static fromError method.

Definitely explicit function, can make it static if needed.

RossBrunton · 2025-04-25T14:29:25Z

offload/unittests/OffloadAPI/kernel/olGetKernel.cpp

+
+TEST_F(olGetKernelTest, InvalidKernelName) {
+  GTEST_SKIP()
+      << "Error code returning from plugin interface not yet supported";


Is skipping the best way of doing this? This test fails, but it fails less badly than before this patch.

You could rename the test to DISABLED_InvalidKernelName to temporarily disable it. Alternatively, you could change the assert to ASSERT_ANY_ERROR which would make it pass for now.

Oh, disabling the test causes it to print "YOU HAVE 1 DISABLED TEST", which is much nicer.

jhuber6

These definitely slip by, thanks for addressing it. Think we could have a utility function that converts an LLVM error into one of the offload ones? In the future ideally we will emit the LLVM errors with the appropriate codes and we can just forward the contained code.

RossBrunton · 2025-04-25T14:48:59Z

These definitely slip by, thanks for addressing it. Think we could have a utility function that converts an LLVM error into one of the offload ones? In the future ideally we will emit the LLVM errors with the appropriate codes and we can just forward the contained code.

I think a utility method makes sense, but that's probably something to do/decide on later once how error codes are going to be passed to offload is more fleshed out.

jhuber6 · 2025-04-25T16:20:27Z

offload/liboffload/src/OffloadImpl.cpp

+  if (!Alloc) {
+    llvm::consumeError(Alloc.takeError());
    return {OL_ERRC_OUT_OF_RESOURCES,
            formatv("Could not create allocation on device {0}", Device).str()};
+  }


Why don't we use the fromError method here?

I only used that function in cases where there was no existing error code. For functions like this, all errors from allocations should result in OL_ERRC_OUT_OF_RESOURCES.

I think it would be much, much better to have a generic method that converts from an LLVM error to an offload error code. We should update all the plugin error values to return some generic error instead of an inconvertable error code. For now, if it's an incovertible error code, just return said generic error. The LLVM error wrapper should be used everywhere up until it's presented to the user as a code + a string.

I've pushed a change which makes all error handling go through fromError. This means that all the error codes from PI are useless at the moment, but once PI gets "proper" error codes it should work better.

Although that does bring up a question... Is it worth having ol_impl_result_t at all? @callumfare any objections to removing it and having the "impl" functions just return llvm::Error directly up until the C boundry?

I added that type so that we could simply return OL_ERRC_FOO or {OL_ERRC_FOO, "Detail string"} from the implementation functions without having to explicitly type out the error conversion each time. It hides the details of how that works from people implementing new entry points. You can see from the definition of ol_impl_result_t that handling the lifetime of the strings and error pointers is a bit ugly. But yeah I wouldn't object to having the impl functions return the llvm::Error and have the handling happen in the wrapper/entry point functions.

We need ol_impl_result_t at the API boundaries because it's a C interface, otherwise it should be consistent with error handling and use the LLVM interface.

ol_result_t is what's actually at the API boundaries (it's a pointer-to-struct). ol_impl_result_t is a helper type that converts to ol_result_t by putting the string and error code in global data structures and returning the pointer to ensure the lifetime is correct.

Ah, well then I agree that we don't need it. However, that can be a separate patch.

jhuber6 · 2025-05-01T15:16:59Z

offload/liboffload/src/OffloadImpl.cpp

+      return ol_impl_result_t::fromError(std::move(Res),
+                                         "The data retrieve operation failed");


The error itself should have a string associated with it, no? Why are we overriding it here and elsewhere.

I've removed the Detail parameter entirely and we just use the Detail from the error message.

`llvm::Error`s containing errors must be explicitly handled or an assert will be raised. With this change, `ol_impl_result_t` can accept and consume an `llvm::Error` for errors raised by PluginInterface. Note that there is currently no facility for PluginInterface to communicate exact error codes, but the constructor is designed in such a way that it can be easily added later. This MR is to convert a crash into an error code. A new test was added, however due to the aforementioned issue with error codes, it does not pass and instead is disabled.

jhuber6

LG, thanks.

jhuber6 · 2025-05-02T12:15:45Z

offload/liboffload/src/OffloadImpl.cpp

@@ -390,13 +389,17 @@ ol_impl_result_t olDestroyEvent_impl(ol_event_handle_t Event) {
 ol_event_handle_t makeEvent(ol_queue_handle_t Queue) {


Probably need this to return an error, but unrelated.

github-actions · 2025-05-02T12:37:41Z

@RossBrunton Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added the offload label Apr 25, 2025

RossBrunton commented Apr 25, 2025

View reviewed changes

jhuber6 reviewed Apr 25, 2025

View reviewed changes

RossBrunton force-pushed the handleerror branch from 1c8b7c3 to e302c82 Compare April 25, 2025 14:46

jhuber6 reviewed Apr 25, 2025

View reviewed changes

RossBrunton force-pushed the handleerror branch 2 times, most recently from 3aec7c2 to e498b79 Compare May 1, 2025 14:41

jhuber6 reviewed May 1, 2025

View reviewed changes

RossBrunton force-pushed the handleerror branch from e498b79 to 18d1426 Compare May 2, 2025 09:01

jhuber6 approved these changes May 2, 2025

View reviewed changes

RossBrunton mentioned this pull request May 2, 2025

[Offload] Add Error Codes to PluginInterface #138258

Open

jhuber6 merged commit f6ac527 into llvm:main May 2, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Offload] Ensure all `llvm::Error`s are handled #137339

[Offload] Ensure all `llvm::Error`s are handled #137339

RossBrunton commented Apr 25, 2025

github-actions bot commented Apr 25, 2025

llvmbot commented Apr 25, 2025

RossBrunton Apr 25, 2025

jhuber6 Apr 25, 2025 •

edited

Loading

RossBrunton Apr 25, 2025

callumfare Apr 25, 2025

RossBrunton Apr 25, 2025

jhuber6 left a comment

RossBrunton commented Apr 25, 2025

jhuber6 Apr 25, 2025

RossBrunton Apr 25, 2025

jhuber6 May 1, 2025 •

edited

Loading

RossBrunton May 1, 2025

callumfare May 1, 2025 •

edited

Loading

jhuber6 May 1, 2025 •

edited

Loading

callumfare May 1, 2025

jhuber6 May 1, 2025

jhuber6 May 1, 2025

RossBrunton May 2, 2025

jhuber6 left a comment

jhuber6 May 2, 2025

github-actions bot commented May 2, 2025

		return ol_impl_result_t::fromError(std::move(Res),
		"The data retrieve operation failed");

		@@ -390,13 +389,17 @@ ol_impl_result_t olDestroyEvent_impl(ol_event_handle_t Event) {
		ol_event_handle_t makeEvent(ol_queue_handle_t Queue) {

[Offload] Ensure all llvm::Errors are handled #137339

[Offload] Ensure all llvm::Errors are handled #137339

Conversation

RossBrunton commented Apr 25, 2025

github-actions bot commented Apr 25, 2025

llvmbot commented Apr 25, 2025

Choose a reason for hiding this comment

jhuber6 Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhuber6 left a comment

Choose a reason for hiding this comment

RossBrunton commented Apr 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhuber6 May 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumfare May 1, 2025 • edited Loading

Choose a reason for hiding this comment

jhuber6 May 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhuber6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2025

[Offload] Ensure all `llvm::Error`s are handled #137339

[Offload] Ensure all `llvm::Error`s are handled #137339

jhuber6 Apr 25, 2025 •

edited

Loading

jhuber6 May 1, 2025 •

edited

Loading

callumfare May 1, 2025 •

edited

Loading

jhuber6 May 1, 2025 •

edited

Loading