Refactor L0 command-buffer cleanup.

[EwanC]

Coverity has reported that there is the possibility of the command-buffer destructor throwing an exception from its calls
to CleanupCompletedEvent.

This is from an underlying throw in GetQueue when a dynamic_cast doesn't behave as expected. I initially tried
changing this to an assert but the V2 Level Zero adapter is triggering this case while in it's developmental state, so an assert brings down the whole test suite rather than just being reported as a fail (the UR loader catches exceptions and returns them as error codes).

Therefore this PR addresses the Coverity report by moving the calls to CleanupCompletedEvent() outside of the command-buffer destructor. This allows us to cleanup prior to calling the destructor, without the risk of throwing an exception.

DPC++ PR intel/llvm#14503

[pbalcer]

E2E L0 failed, but it doesn't seem related:

Failed Tests (1):
  SYCL :: ESIMD/regression/minmax.cpp

[pbalcer]

lgtm, but this is quite a lot of changes to handle a fairly trivial problem.

@igchor it may make sense to create a nothrow variant of the getQueue method that returns something like an std::optional if succesful. Thoughts?

[EwanC]

@igchor it may make sense to create a nothrow variant of the getQueue method that returns something like an std::optional if succesful. Thoughts?

ping @igchor, I could try make this change instead if you think it's a better way to go

[igchor]

@igchor it may make sense to create a nothrow variant of the getQueue method that returns something like an std::optional if succesful. Thoughts?

ping @igchor, I could try make this change instead if you think it's a better way to go

Sorry, I missed the notification.

I think it would be best to just move all the logic from the destructor to a function like you did but not using UR_CALL_STORE_ERR inside - leaving all the logic as is. Also, the function can just return void.

If an exception from GetQueue is thrown, (.e.g from urDeviceRelease) the rest of the cleanup functions won't be called anyway because the exception will abort the cleanupCommandBufferResources function and propagate up. To avoid that, you would have to wrap each function in a try/catch but I don't think that's a good idea (you might end up with some inconsistent state - it's better to just have a leak). Also, as far as I understand this should not happen during execution anyway, it is just something coverity thinks is possible, right?

[igchor]

As for a nothrow variant of GetQueue - you would still need to handle nullopt and propagate the error somehow, I'm not sure if it would be easier.

[EwanC]

Thanks for your reply @igchor, I've updated the PR with your suggestion.

It is indeed only the possibility of an exception being thrown in a C++ object destructor that Coverity has detected. Avoiding leaks and returning a reliable error when things go wrong when freeing resources is over an above that, so can be considered out of scope for the purposes of this PR.