Merge pull request #2327 from hdelan/double-finalize

[CommandBuffer] Make double finalizing fail

Author: martygrant

include/ur_api.h

@@ -8562,6 +8562,7 @@ urCommandBufferReleaseExp(
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_HANDLE
 ///         + `NULL == hCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_COMMAND_BUFFER_EXP
+///     - ::UR_RESULT_ERROR_INVALID_OPERATION - "If `hCommandBuffer` has already been finalized"
 ///     - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
 ///     - ::UR_RESULT_ERROR_OUT_OF_RESOURCES
 UR_APIEXPORT ur_result_t UR_APICALL

scripts/core/exp-command-buffer.yml

@@ -330,6 +330,8 @@ params:
       desc: "[in] Handle of the command-buffer object."
 returns:
     - $X_RESULT_ERROR_INVALID_COMMAND_BUFFER_EXP
+    - $X_RESULT_ERROR_INVALID_OPERATION
+        - "If `hCommandBuffer` has already been finalized"
     - $X_RESULT_ERROR_OUT_OF_HOST_MEMORY
     - $X_RESULT_ERROR_OUT_OF_RESOURCES
 --- #--------------------------------------------------------------------------

source/adapters/cuda/command_buffer.cpp

@@ -404,6 +404,8 @@ urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
 
 UR_APIEXPORT ur_result_t UR_APICALL
 urCommandBufferFinalizeExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
+  UR_ASSERT(hCommandBuffer->CudaGraphExec == nullptr,
+            UR_RESULT_ERROR_INVALID_OPERATION);
   try {
     const unsigned long long flags = 0;
 #if CUDA_VERSION >= 12000

source/adapters/cuda/command_buffer.hpp

@@ -355,7 +355,7 @@ struct ur_exp_command_buffer_handle_t_ {
   // Cuda Graph handle
   CUgraph CudaGraph;
   // Cuda Graph Exec handle
-  CUgraphExec CudaGraphExec;
+  CUgraphExec CudaGraphExec = nullptr;
   // Atomic variable counting the number of reference to this command_buffer
   // using std::atomic prevents data race when incrementing/decrementing.
   std::atomic_uint32_t RefCountInternal;

source/adapters/hip/command_buffer.cpp

@@ -304,6 +304,8 @@ urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
 
 UR_APIEXPORT ur_result_t UR_APICALL
 urCommandBufferFinalizeExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
+  UR_ASSERT(hCommandBuffer->HIPGraphExec == nullptr,
+            UR_RESULT_ERROR_INVALID_OPERATION);
   try {
     const unsigned long long flags = 0;
     UR_CHECK_ERROR(hipGraphInstantiateWithFlags(

source/adapters/hip/command_buffer.hpp

@@ -175,7 +175,7 @@ struct ur_exp_command_buffer_handle_t_ {
   // HIP Graph handle
   hipGraph_t HIPGraph;
   // HIP Graph Exec handle
-  hipGraphExec_t HIPGraphExec;
+  hipGraphExec_t HIPGraphExec = nullptr;
   // Atomic variable counting the number of reference to this command_buffer
   // using std::atomic prevents data race when incrementing/decrementing.
   std::atomic_uint32_t RefCountInternal;

source/adapters/level_zero/command_buffer.cpp

@@ -865,6 +865,7 @@ finalizeWaitEventPath(ur_exp_command_buffer_handle_t CommandBuffer) {
 ur_result_t
 urCommandBufferFinalizeExp(ur_exp_command_buffer_handle_t CommandBuffer) {
   UR_ASSERT(CommandBuffer, UR_RESULT_ERROR_INVALID_NULL_POINTER);
+  UR_ASSERT(!CommandBuffer->IsFinalized, UR_RESULT_ERROR_INVALID_OPERATION);
 
   // It is not allowed to append to command list from multiple threads.
   std::scoped_lock<ur_shared_mutex> Guard(CommandBuffer->Mutex);

source/adapters/opencl/command_buffer.cpp

@@ -124,6 +124,7 @@ urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
 
 UR_APIEXPORT ur_result_t UR_APICALL
 urCommandBufferFinalizeExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
+  UR_ASSERT(!hCommandBuffer->IsFinalized, UR_RESULT_ERROR_INVALID_OPERATION);
   cl_context CLContext = cl_adapter::cast<cl_context>(hCommandBuffer->hContext);
   cl_ext::clFinalizeCommandBufferKHR_fn clFinalizeCommandBufferKHR = nullptr;
   UR_RETURN_ON_FAILURE(

source/loader/ur_libapi.cpp

@@ -7584,6 +7584,7 @@ ur_result_t UR_APICALL urCommandBufferReleaseExp(
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_HANDLE
 ///         + `NULL == hCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_COMMAND_BUFFER_EXP
+///     - ::UR_RESULT_ERROR_INVALID_OPERATION - "If `hCommandBuffer` has already been finalized"
 ///     - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
 ///     - ::UR_RESULT_ERROR_OUT_OF_RESOURCES
 ur_result_t UR_APICALL urCommandBufferFinalizeExp(

source/ur_api.cpp

@@ -6440,6 +6440,7 @@ ur_result_t UR_APICALL urCommandBufferReleaseExp(
 ///     - ::UR_RESULT_ERROR_INVALID_NULL_HANDLE
 ///         + `NULL == hCommandBuffer`
 ///     - ::UR_RESULT_ERROR_INVALID_COMMAND_BUFFER_EXP
+///     - ::UR_RESULT_ERROR_INVALID_OPERATION - "If `hCommandBuffer` has already been finalized"
 ///     - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
 ///     - ::UR_RESULT_ERROR_OUT_OF_RESOURCES
 ur_result_t UR_APICALL urCommandBufferFinalizeExp(

test/conformance/exp_command_buffer/commands.cpp

@@ -204,3 +204,14 @@ TEST_P(urCommandBufferAppendKernelLaunchExpTest, Basic) {
         ASSERT_EQ(result, ptrZ[i]);
     }
 }
+
+TEST_P(urCommandBufferAppendKernelLaunchExpTest, FinalizeTwice) {
+    ASSERT_SUCCESS(urCommandBufferAppendKernelLaunchExp(
+        cmd_buf_handle, kernel, n_dimensions, &global_offset, &global_size,
+        &local_size, 0, nullptr, 0, nullptr, 0, nullptr, nullptr, nullptr,
+        nullptr));
+
+    ASSERT_SUCCESS(urCommandBufferFinalizeExp(cmd_buf_handle));
+    EXPECT_EQ_RESULT(urCommandBufferFinalizeExp(cmd_buf_handle),
+                     UR_RESULT_ERROR_INVALID_OPERATION);
+}

test/conformance/exp_command_buffer/exp_command_buffer_adapter_level_zero_v2.match

@@ -14,6 +14,7 @@ urCommandBufferCommandsTest.urCommandBufferAppendMemBufferFillExp/*
 urCommandBufferCommandsTest.urCommandBufferAppendUSMPrefetchExp/*
 urCommandBufferCommandsTest.urCommandBufferAppendUSMAdviseExp/*
 urCommandBufferAppendKernelLaunchExpTest.Basic/*
+urCommandBufferAppendKernelLaunchExpTest.FinalizeTwice/*
 urCommandBufferFillCommandsTest.Buffer/*
 urCommandBufferFillCommandsTest.USM/*
 KernelCommandEventSyncTest.Basic/*

test/conformance/exp_command_buffer/exp_command_buffer_adapter_native_cpu.match

@@ -5,6 +5,7 @@
 {{OPT}}urCommandBufferRetainCommandExpTest.Success/*
 {{OPT}}urCommandBufferRetainCommandExpTest.InvalidNullHandle/*
 {{OPT}}urCommandBufferAppendKernelLaunchExpTest.Basic/*
+{{OPT}}urCommandBufferAppendKernelLaunchExpTest.FinalizeTwice/*
 {{OPT}}BufferFillCommandTest.UpdateParameters/*
 {{OPT}}BufferFillCommandTest.UpdateGlobalSize/*
 {{OPT}}BufferFillCommandTest.SeparateUpdateCalls/*