[SYCL] Add missed checks of ur call result in commands.cpp (#16748)

ur calls in enqueueImp should return error via return value
(adapter->call_nocheck), not exception (adapter->call).
ur_event_handle in corresponding event_impl also should be set only in
case of successful call.
Some functions have already had this logic. This PR fixes the rest.

---------

Signed-off-by: Tikhomirova, Kseniya <[email protected]>

Author: KseniyaTikhomirova

sycl/source/detail/scheduler/commands.cpp

@@ -2525,7 +2525,7 @@ static ur_result_t SetKernelParamsAndLaunch(
             property_list.size(), property_list.data(), RawEvents.size(),
             RawEvents.empty() ? nullptr : &RawEvents[0],
             OutEventImpl ? &UREvent : nullptr);
-    if (OutEventImpl) {
+    if ((Error == UR_RESULT_SUCCESS) && OutEventImpl) {
       OutEventImpl->setHandle(UREvent);
     }
     return Error;
@@ -3421,15 +3421,21 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
 
     ur_bool_t NativeCommandSupport = false;
     assert(MQueue && "Native command should have an associated queue");
-    MQueue->getAdapter()->call<UrApiKind::urDeviceGetInfo>(
+    auto &Adapter = MQueue->getAdapter();
+    Adapter->call<UrApiKind::urDeviceGetInfo>(
         detail::getSyclObjImpl(MQueue->get_device())->getHandleRef(),
         UR_DEVICE_INFO_ENQUEUE_NATIVE_COMMAND_SUPPORT_EXP,
         sizeof(NativeCommandSupport), &NativeCommandSupport, nullptr);
     assert(NativeCommandSupport && "ext_codeplay_enqueue_native_command is not "
                                    "supported on this device");
-    MQueue->getAdapter()->call<UrApiKind::urEnqueueNativeCommandExp>(
-        MQueue->getHandleRef(), InteropFreeFunc, &CustomOpData, ReqMems.size(),
-        ReqMems.data(), nullptr, RawEvents.size(), RawEvents.data(), Event);
+    if (auto Result =
+            Adapter->call_nocheck<UrApiKind::urEnqueueNativeCommandExp>(
+                MQueue->getHandleRef(), InteropFreeFunc, &CustomOpData,
+                ReqMems.size(), ReqMems.data(), nullptr, RawEvents.size(),
+                RawEvents.data(), Event);
+        Result != UR_RESULT_SUCCESS)
+      return Result;
+
     SetEventHandleOrDiscard();
     return UR_RESULT_SUCCESS;
   }
@@ -3449,8 +3455,12 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     const AdapterPtr &Adapter = MQueue->getAdapter();
     if (MEvent != nullptr)
       MEvent->setHostEnqueueTime();
-    Adapter->call<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
-        MQueue->getHandleRef(), &Properties, 0, nullptr, Event);
+    if (auto Result =
+            Adapter->call_nocheck<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
+                MQueue->getHandleRef(), &Properties, 0, nullptr, Event);
+        Result != UR_RESULT_SUCCESS)
+      return Result;
+
     SetEventHandleOrDiscard();
     return UR_RESULT_SUCCESS;
   }
@@ -3479,9 +3489,13 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     const AdapterPtr &Adapter = MQueue->getAdapter();
     if (MEvent != nullptr)
       MEvent->setHostEnqueueTime();
-    Adapter->call<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
-        MQueue->getHandleRef(), &Properties, UrEvents.size(), &UrEvents[0],
-        Event);
+    if (auto Result =
+            Adapter->call_nocheck<UrApiKind::urEnqueueEventsWaitWithBarrierExt>(
+                MQueue->getHandleRef(), &Properties, UrEvents.size(),
+                &UrEvents[0], Event);
+        Result != UR_RESULT_SUCCESS)
+      return Result;
+
     SetEventHandleOrDiscard();
     return UR_RESULT_SUCCESS;
   }
@@ -3493,6 +3507,10 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     ur_event_handle_t *TimestampDeps = nullptr;
     size_t NumTimestampDeps = 0;
 
+    // TO DO - once the following WA removed: to change call to call_nocheck and
+    // return operation result to Command::enqueue (see other CG types). Set
+    // UREvent to EventImpl only for successful case.
+
     // If the queue is not in-order, the implementation will need to first
     // insert a marker event that the timestamp waits for.
     ur_event_handle_t PreTimestampMarkerEvent{};
@@ -3581,15 +3599,18 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
         static_cast<CGExecCommandBuffer *>(MCommandGroup.get());
     if (MEvent != nullptr)
       MEvent->setHostEnqueueTime();
-    ur_result_t Err =
-        MQueue->getAdapter()
-            ->call_nocheck<UrApiKind::urCommandBufferEnqueueExp>(
-                CmdBufferCG->MCommandBuffer, MQueue->getHandleRef(),
-                RawEvents.size(), RawEvents.empty() ? nullptr : &RawEvents[0],
-                Event);
+    if (auto Result =
+            MQueue->getAdapter()
+                ->call_nocheck<UrApiKind::urCommandBufferEnqueueExp>(
+                    CmdBufferCG->MCommandBuffer, MQueue->getHandleRef(),
+                    RawEvents.size(),
+                    RawEvents.empty() ? nullptr : &RawEvents[0], Event);
+        Result != UR_RESULT_SUCCESS)
+      return Result;
+
     SetEventHandleOrDiscard();
 
-    return Err;
+    return UR_RESULT_SUCCESS;
   }
   case CGType::CopyImage: {
     CGCopyImage *Copy = (CGCopyImage *)MCommandGroup.get();
@@ -3614,11 +3635,11 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     const detail::AdapterPtr &Adapter = MQueue->getAdapter();
     auto OptWaitValue = SemWait->getWaitValue();
     uint64_t WaitValue = OptWaitValue.has_value() ? OptWaitValue.value() : 0;
-    Adapter->call<UrApiKind::urBindlessImagesWaitExternalSemaphoreExp>(
-        MQueue->getHandleRef(), SemWait->getExternalSemaphore(),
-        OptWaitValue.has_value(), WaitValue, 0, nullptr, nullptr);
 
-    return UR_RESULT_SUCCESS;
+    return Adapter
+        ->call_nocheck<UrApiKind::urBindlessImagesWaitExternalSemaphoreExp>(
+            MQueue->getHandleRef(), SemWait->getExternalSemaphore(),
+            OptWaitValue.has_value(), WaitValue, 0, nullptr, nullptr);
   }
   case CGType::SemaphoreSignal: {
     assert(MQueue &&
@@ -3628,11 +3649,10 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     auto OptSignalValue = SemSignal->getSignalValue();
     uint64_t SignalValue =
         OptSignalValue.has_value() ? OptSignalValue.value() : 0;
-    Adapter->call<UrApiKind::urBindlessImagesSignalExternalSemaphoreExp>(
-        MQueue->getHandleRef(), SemSignal->getExternalSemaphore(),
-        OptSignalValue.has_value(), SignalValue, 0, nullptr, nullptr);
-
-    return UR_RESULT_SUCCESS;
+    return Adapter
+        ->call_nocheck<UrApiKind::urBindlessImagesSignalExternalSemaphoreExp>(
+            MQueue->getHandleRef(), SemSignal->getExternalSemaphore(),
+            OptSignalValue.has_value(), SignalValue, 0, nullptr, nullptr);
   }
   case CGType::None: {
     if (RawEvents.empty()) {
@@ -3644,11 +3664,14 @@ ur_result_t ExecCGCommand::enqueueImpQueue() {
     assert(MQueue && "Empty node should have an associated queue");
     const detail::AdapterPtr &Adapter = MQueue->getAdapter();
     ur_event_handle_t Event;
-    ur_result_t Result = Adapter->call_nocheck<UrApiKind::urEnqueueEventsWait>(
-        MQueue->getHandleRef(), RawEvents.size(),
-        RawEvents.size() ? &RawEvents[0] : nullptr, &Event);
+    if (auto Result = Adapter->call_nocheck<UrApiKind::urEnqueueEventsWait>(
+            MQueue->getHandleRef(), RawEvents.size(),
+            RawEvents.size() ? &RawEvents[0] : nullptr, &Event);
+        Result != UR_RESULT_SUCCESS)
+      return Result;
+
     MEvent->setHandle(Event);
-    return Result;
+    return UR_RESULT_SUCCESS;
   }
   }
   return UR_RESULT_ERROR_INVALID_OPERATION;

sycl/unittests/scheduler/FailedCommands.cpp

@@ -86,3 +86,77 @@ TEST_F(SchedulerTest, FailedCopyBackException) {
                                            &failingUrCall);
   RunWithFailedCommandsAndCheck(false, 1);
 }
+
+bool DummyEventReturned = false;
+bool DummyEventReleaseAttempt = false;
+ur_event_handle_t DummyEvent = mock::createDummyHandle<ur_event_handle_t>();
+
+inline ur_result_t failedEnqueueKernelLaunchWithDummy(void *pParams) {
+  DummyEventReturned = true;
+  auto params = *static_cast<ur_enqueue_kernel_launch_params_t *>(pParams);
+  **params.pphEvent = DummyEvent;
+  return UR_RESULT_ERROR_UNKNOWN;
+}
+
+inline ur_result_t checkDummyInEventRelease(void *pParams) {
+  auto params = static_cast<ur_event_handle_t>(pParams);
+  DummyEventReleaseAttempt = params == DummyEvent;
+  return UR_RESULT_SUCCESS;
+}
+
+inline ur_result_t failedEnqueueBarrierWithDummy(void *pParams) {
+  DummyEventReturned = true;
+  auto params =
+      *static_cast<ur_enqueue_events_wait_with_barrier_ext_params_t *>(pParams);
+  **params.pphEvent = DummyEvent;
+  return UR_RESULT_ERROR_UNKNOWN;
+}
+
+// Checks that in case of failed command and "valid" event assigned to output
+// event var, RT ignores it and do not call release since its usage is undefined
+// behavior.
+TEST(FailedCommandsTest, CheckUREventReleaseWithKernel) {
+  DummyEventReleaseAttempt = false;
+  DummyEventReturned = false;
+  sycl::unittest::UrMock<> Mock;
+  mock::getCallbacks().set_before_callback("urEnqueueKernelLaunch",
+                                           &failedEnqueueKernelLaunchWithDummy);
+  mock::getCallbacks().set_before_callback("urEventRelease",
+                                           &checkDummyInEventRelease);
+  platform Plt = sycl::platform();
+  queue Queue(context(Plt), default_selector_v);
+  {
+    try {
+      Queue.submit(
+          [&](sycl::handler &CGH) { CGH.single_task<TestKernel<1>>([]() {}); });
+    } catch (...) {
+    }
+  }
+  Queue.wait();
+  ASSERT_TRUE(DummyEventReturned);
+  ASSERT_FALSE(DummyEventReleaseAttempt);
+}
+
+// Checks that in case of failed command and "valid" event assigned to output
+// event var, RT ignores it and do not call release since its usage is undefined
+// behavior.
+TEST(FailedCommandsTest, CheckUREventReleaseWithBarrier) {
+  DummyEventReleaseAttempt = false;
+  DummyEventReturned = false;
+  sycl::unittest::UrMock<> Mock;
+  mock::getCallbacks().set_before_callback("urEnqueueEventsWaitWithBarrierExt",
+                                           &failedEnqueueBarrierWithDummy);
+  mock::getCallbacks().set_before_callback("urEventRelease",
+                                           &checkDummyInEventRelease);
+  platform Plt = sycl::platform();
+  queue Queue(context(Plt), default_selector_v);
+  {
+    try {
+      Queue.submit([&](sycl::handler &CGH) { CGH.ext_oneapi_barrier(); });
+    } catch (...) {
+    }
+  }
+  Queue.wait();
+  ASSERT_TRUE(DummyEventReturned);
+  ASSERT_FALSE(DummyEventReleaseAttempt);
+}