Skip to content

Add error handling - #28

Open
szymon-zadworny wants to merge 8 commits into
oneapi-src:mainfrom
szymon-zadworny:error-handling
Open

Add error handling#28
szymon-zadworny wants to merge 8 commits into
oneapi-src:mainfrom
szymon-zadworny:error-handling

Conversation

@szymon-zadworny

Copy link
Copy Markdown
Contributor

This PR adds synchronous SYCL exception support to:

  • queues
  • events
  • kernel
    This also changes the Buffer API, since initialization requires waiting on an event.

The PR also updates the examples.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces synchronous SYCL exception propagation into the safe Rust API by threading Result<T, cxx::Exception> through key SYCL operations (queue barriers/waits/launches, kernel bundle build/lookups, and event waits), and updates buffer initialization and examples to reflect the now-fallible workflow.

Changes:

  • Add crate-level SyclError / Result<T> aliases and update multiple APIs to return Result (queue, event, kernel bundle, nd-range launch).
  • Update EventFuture and BufferFuture/EnqueuedBuffer to yield Result so async usage can propagate SYCL exceptions.
  • Update examples to use ? with fallible queue/buffer/kernel operations.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
oneapi-rs/src/lib.rs Defines public SyclError and Result<T> aliases used across the crate.
oneapi-rs/src/queue.rs Makes barrier/wait/launch fallible (Result) to surface synchronous SYCL exceptions.
oneapi-rs/src/range.rs Updates ND-range launch trait/impls to return Result<Event>.
oneapi-rs/src/event.rs Makes Event::wait() and EventFuture return Result<()> for exception propagation in async flows.
oneapi-rs/src/context.rs Makes kernel bundle creation from source fallible.
oneapi-rs/src/kernel.rs Makes kernel bundle build and kernel lookup fallible.
oneapi-rs/src/buffer.rs Makes buffer initialization waiting/awaiting return Result<Buffer<…>>.
oneapi-rs/examples/kernel_launch.rs Updates async example to propagate errors via ? and return Result<()>.
oneapi-rs/examples/kernel_launch_derive.rs Updates sync example to propagate errors via ? and return Result<()>.
oneapi-rs-sys/src/queue-sys.rs Updates selected queue FFI bindings (barrier/wait/launch) to return Result for C++ exception capture.
oneapi-rs-sys/src/event-sys.rs Updates event wait FFI binding to return Result<()> for C++ exception capture.
oneapi-rs-sys/src/kernel-bundle-sys.rs Updates kernel-bundle FFI bindings to return Result for C++ exception capture.
Suppressed comments (2)

oneapi-rs-sys/src/event-sys.rs:33

  • register_callback is declared as infallible (unsafe fn ... -> ()), but the C++ implementation calls queue->submit(...) (see oneapi-rs-sys/src/event.cpp:37), which can throw sycl::exception. If an exception is thrown here it will unwind across the FFI boundary and likely abort/UB instead of being surfaced as a Rust error.

Consider changing the bridge signature to return Result<()> (so cxx catches C++ exceptions) and then propagating/handling the error in oneapi-rs/src/event.rs when setting the callback.

        fn wait(event: &mut UniquePtr<Event>) -> Result<()>;
        unsafe fn register_callback(
            queue: &mut UniquePtr<Queue>,
            event: &Event,
            waker: *const SharedWaker,
        );

oneapi-rs-sys/src/queue-sys.rs:81

  • memset and memcpy are still declared as infallible (-> UniquePtr<Event>), but their C++ implementations call queue->memset(...) / queue->memcpy(...) (see oneapi-rs-sys/src/queue.cpp:41 and :100), which can throw sycl::exception synchronously. As written, any thrown exception would unwind across the FFI boundary.

Consider updating these to return Result<UniquePtr<Event>> (matching barrier/wait/launch_*) and then adjusting the safe Rust wrappers in oneapi-rs/src/queue.rs accordingly (e.g., memset_with_deps / copy_with_deps should return Result<Event>).

        ) -> Result<UniquePtr<Event>>;

        unsafe fn memcpy(
            queue: &mut UniquePtr<Queue>,
            dest: *mut u8,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bratpiorka bratpiorka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved but please consider if Copilot suppressed comments are valid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants