[SYCL] Fix coverity issues 11/11 2025

[steffenlarsen]

This commit makes the following changes to sate Coverity:

1. Initializes uninitialized variables. This mainly affects UR handles that are expected to be set after their corresponding UR calls.
2. Catch exceptions in dtors, similar to how we do for *_impl classes.
3. Fix use-after-move cases and set more appropriate ownerships.
4. Change a selection of arguments to const references.

[vinser52]

Now we have a copy instead of a move. Does Coverity report potential use after the move? If so, does actual use occur after the move? If no, I believe the proper fix should be to change the submit_kernel_scheduler_bypass function to accept DepEvents by value and move the vector when we call the submit_kernel_scheduler_bypass.

[steffenlarsen]

Does Coverity report potential use after the move?

Yes.

If so, does actual use occur after the move?

It is hard to say. The issue is that the events are actually owned by another object further up. This function is effectively stealing the dependency events from the object, without that behavior being very clear. Even if use-after-move isn't present currently, it is going to be very easy for someone to not realize it has been moved and assume that the dependencies tied to the owner object are still valid.

If no, I believe the proper fix should be to change the submit_kernel_scheduler_bypass function to accept DepEvents by value and move the vector when we call the submit_kernel_scheduler_bypass.

I disagree. In the current solution, a copy only happens under certain conditions. If we change the function to take it by value we will always make a copy of the dependencies vector.

[vinser52]

I disagree. In the current solution, a copy only happens under certain conditions. If we change the function to take it by value we will always make a copy of the dependencies vector.

The problem I see right now is that before this PR, we had a move in case of a certain condition. And now we will have a copy.

What I have in mind is that today we have something like this:

void foo(std::vector<event> &DepEvents) {
   // ...
   ResultEvent->getPreparedDepsEvents() = std::move(DepEvents);
}

void bar() {
    std::vector<event> events;
    ...

    // It is hard to control what foo will do with events when we pass it by reference
    foo(events);
}

And I suggested considering the following:

void foo(std::vector<event> DepEvents) {
   // ...
   ResultEvent->getPreparedDepsEvents() = std::move(DepEvents);
}

void bar() {
    std::vector<event> events;
    ...

    // we explicitly do move and should not care how foo process the events.
    foo(std::move(events));
}

@slawekptak, please review.

[slawekptak]

I think the second version with double std::move should work - there is no copy and the ownership is clear.

[steffenlarsen]

Discussed this offline with @vinser52 and we agreed that doing the by-value solution can cause unnecessary copies in certain paths. As such, I plan on refactoring this code and its callers slightly to make the ownership easier to track.

[vinser52]

why did you remove the std::move(KData).getArgs()?

[steffenlarsen]

KData is used in the following arguments. That's a use-after-move.

[vinser52]

but we do not move the KData itself here. Such syntax forces calling of the right overload of getArgs() that does the move of args. With your change it will be a copy instead of move.

[steffenlarsen]

Ah, I was not aware of that syntax... And I suppose neither is Coverity. 😆

I still have concerns regarding it, but I can revert it here and we can discuss it in #20617 (comment).