[UR] Coverity fixes - #22699
Conversation
099bdeb to
5a27119
Compare
5a27119 to
bc2adf7
Compare
bc2adf7 to
dcbf84e
Compare
| Result = EnqueueUSMSetZero(hQueue, (void *)ShadowAddr, size, | ||
| numEventsInWaitList, phEventWaitList, &Event); | ||
| if (Result != UR_RESULT_SUCCESS) { | ||
| releaseEvent(Event); |
There was a problem hiding this comment.
Event is in an undefined state here if Result is not success.
There was a problem hiding this comment.
ok I will mark MSAN CIDs as false positives in Coverity
There was a problem hiding this comment.
... but I'n not sure about this. Looking at https://github.com/intel/llvm/blob/sycl/unified-runtime/source/adapters/opencl/usm.cpp#L344 the event ptr is filled with data and in https://github.com/intel/llvm/blob/sycl/unified-runtime/source/adapters/opencl/usm.cpp#L365 the function returns without releasing the event pointer, so there could be an actual memory leak in this scenario
There was a problem hiding this comment.
That's an adapter issue. Out events shouldn't be allocated if the call fails.
| if (Result != UR_RESULT_SUCCESS || Type == DeviceType::CPU) { | ||
| [[maybe_unused]] auto ReleaseResult = | ||
| getContext()->urDdiTable.Context.pfnRelease(ShadowContext); | ||
| assert(ReleaseResult == UR_RESULT_SUCCESS); | ||
| } |
There was a problem hiding this comment.
did this just leak on the cpu type before?
There was a problem hiding this comment.
Yes, CPU shadow context Destroy() function does not release this context. For GPU type it is done in ShadowMemoryGPU::Destroy()
| try { | ||
| cl_int Res = | ||
| cl_ext::getExtFuncFromContext<decltype(clReleaseCommandBufferKHR)>( | ||
| CLContext, | ||
| cast(ur::cl::getAdapter())->fnCache.clReleaseCommandBufferKHRCache, | ||
| cl_ext::ReleaseCommandBufferName, &clReleaseCommandBufferKHR); | ||
| assert(Res == CL_SUCCESS); | ||
| (void)Res; | ||
| } catch (...) { | ||
| assert(false && "Failed to look up clReleaseCommandBufferKHR"); | ||
| } |
There was a problem hiding this comment.
It would be cleaner to have it already stored somewhere.
There was a problem hiding this comment.
done - I stored create and release in ur_exp_command_buffer_handle_t_
| { | ||
| std::lock_guard lock{Platform->SubDevicesLock}; | ||
| Platform->SubDevices.erase(CLDevice); | ||
| } |
There was a problem hiding this comment.
was the previous comment incorrect?
There was a problem hiding this comment.
Coverity claims that there could be a race condition here if another thread concurrently performs a lookup or insertion
| if (!SetKernelArgMemPointerPtr) { | ||
| UR_RETURN_ON_FAILURE( | ||
| cl_ext::getExtFuncFromContext<clSetKernelArgMemPointerINTEL_fn>( | ||
| Queue->Context->CLContext, | ||
| cast(ur::cl::getAdapter()) | ||
| ->fnCache.clSetKernelArgMemPointerINTELCache, | ||
| cl_ext::SetKernelArgMemPointerName, | ||
| &SetKernelArgMemPointerPtr)); | ||
| } |
There was a problem hiding this comment.
I'd rather keep this function pointer in the context or something.
There was a problem hiding this comment.
I think that L0 adapters store it in the platform structure.
There was a problem hiding this comment.
ok I used Kernel->clSetKernelArgMemPointerINTEL
| if (!SetKernelArgMemPointerPtr) { | ||
| UR_RETURN_ON_FAILURE( | ||
| cl_ext::getExtFuncFromContext<clSetKernelArgMemPointerINTEL_fn>( | ||
| Queue->Context->CLContext, | ||
| cast(ur::cl::getAdapter()) | ||
| ->fnCache.clSetKernelArgMemPointerINTELCache, | ||
| cl_ext::SetKernelArgMemPointerName, | ||
| &SetKernelArgMemPointerPtr)); | ||
| } |
There was a problem hiding this comment.
I think that L0 adapters store it in the platform structure.
dcbf84e to
33d3916
Compare
Release the context when shadow setup fails. CPU shadow memory is a singleton and does not retain per-device contexts, so release those after setup as well.
33d3916 to
1b8cfd5
Compare
Protect SubDevices erasure with SubDevicesLock. Fixes Coverity CID 3644020.
Validate device-dependent allocations before calling the DDI. Fixes Coverity CID 3644040.
mmichel11
left a comment
mmichel11
left a comment
There was a problem hiding this comment.
Command buffer changes LGTM
|
native CPU failure is unrelated: #22850 |
4 participants
fixes for Coverity issues: 3482570, 3491402, 3491404, 3491405, 3643292, 3643293, 3643294, 3643295, 3644020, 3644025, 3644034, 3644037, 3644040