-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault #2506
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault #2506
Conversation
m_KernelMap.emplace(Kernel, | ||
std::make_shared<KernelInfo>(Kernel, IsInstrumented)); | ||
return UR_RESULT_SUCCESS; | ||
std::make_unique<KernelInfo>(Kernel, IsInstrumented)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother using a unique_ptr
if there is no ownership change? We can just use raw pointer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to manually delete them if it's raw pointer. unique_ptr
is more convenient here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can just use the object created by the map. It should be deleted when being erased from then map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can just use the object created by the map. It should be deleted when being erased from then map.
I've tried this but encountered a difficult-to-solve compilation error.
Using unqiue_ptr
is a simple solution here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I am fine with this as a temporary solution. We plan to refactor related code in near future anyway. We can discuss for any better solution in that time.
@oneapi-src/unified-runtime-maintain, please add to 2025.1 |
Hi @zhaomaosu , please help review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Changes pulled in by intel/llvm#16478. |
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault
[DeviceASAN] Fix urKernelCreateWithNativeHandle segfault
LLVM: intel/llvm#16473
Since we can't get nullptr when calling
getKernelInfo
, I changed the return type of this function to reference.To make it support "urKernelCreateXXX" APIs automatically, I changed
getKernelInfo
togetOrCreateKernelInfo
, so that we needn't intercept "urKernelCreateXXX" anymore.Sync those changes to msan.