[RFC] Fixing dangling pointers caused by urCommandBufferRelease #1898
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
loader
kbenzie
reviewed
Jul 26, 2024
| @@ -8314,7 +8314,7 @@ urCommandBufferRetainExp( | |||
| /// - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY | |||
| UR_APIEXPORT ur_result_t UR_APICALL | |||
| urCommandBufferReleaseExp( | |||
| ur_exp_command_buffer_handle_t hCommandBuffer ///< [in][release] Handle of the command-buffer object. | |||
| ur_exp_command_buffer_handle_t &hCommandBuffer ///< [in][release] Handle of the command-buffer object. | |||
There was a problem hiding this comment.
This header is a C API, there can not be use of C++ references in it. Additionally, this header is generated from a spec description in yaml files which reside in scripts/core/*.yml.
| if (!CommandBuffer->RefCount.decrementAndTest()) | ||
| return UR_RESULT_SUCCESS; | ||
|
|
||
| delete CommandBuffer; | ||
| CommandBuffer = nullptr; |
There was a problem hiding this comment.
It is not the responsibility of the level zero adapter to zero out the handle pointer as part of the urCommandBufferReleaseExp entry point. The proper fix for this should be in the layer above which is calling this entry point.
|
With the removal of the PI layer in intel/llvm#14145, there would no longer be a need for such mechanism. Thus, I am closing this. Thanks for your time and patience regardless! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some functions in the UR leaves dangling pointers, such as
commandBufferReleaseInternal, which is called byurCommandBufferReleaseExp. Coverity has detected that on intel/llvm, whenurCommandBufferReleaseExpis called from the pi with tracing enabled, dangling pointers left byurCommandBufferReleaseExpare used again here (specifically, thepi_ext_command_buffer/ur_exp_command_buffer_handle_tfed intoArgs, which was already freed inurCommandBufferRelease).The best course of action to fix this Coverity hit, to the best of my knowledge, is to fix the dangling pointers left by e.g.
urCommandBufferReleaseExp. This PR contains a potential fix by taking references ofur_exp_command_buffer_handle_twhen callingurCommandBufferReleasesinstead, and setting the pointers tonullptrafter they are deleted. This would allow code usingurCommandBufferReleaseExpto actually catch that the command buffer has been freed.Unfortunately, this requires the API to be changed, as I've changed the function signature of
urCommandBufferReleaseExp. I'm also aware that changing onlyurCommandBufferReleasemay result in asymmetry within the API. Thus, if this change was to be adopted, more functions may also have to be changed in a similar manner. Thus, I am opening this PR as an RFC to see what other people think of this, and to see if there are any potential issues with this approach.Additionally, if anyone has better ideas for fixing/handling this issue/Coverity hit without changing the UR API, please feel free to let me know. From what I can tell, this is the course of action that'll result in the least amount of problems in the future.
The respective draft DPC++ testing PR is here: intel/llvm#14782