Skip to content
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

[DeviceSanitizer] Support "-fsanitize-ignorelist=" to disable sanitizing on some of kernels #2055

Merged
merged 23 commits into from
Nov 26, 2024

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Sep 5, 2024

LLVM: intel/llvm#15294

Add a device global "__AsanKernelMetadata" which records the sanitized kernel name, so that we can check if sanitizer layer needs specially handle it.

@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Sep 5, 2024
@AllanZyne AllanZyne marked this pull request as ready for review September 6, 2024 06:15
@AllanZyne AllanZyne requested a review from a team as a code owner September 6, 2024 06:15
@pbalcer
Copy link
Contributor

pbalcer commented Oct 28, 2024

please rebase

@AllanZyne AllanZyne changed the title [DeviceSanitizer] Kernel name filter [DeviceSanitizer] Support "-fsanitize-ignorelist=" to disable sanitizing on some of kernels Nov 7, 2024
@AllanZyne AllanZyne marked this pull request as draft November 21, 2024 07:40
@AllanZyne AllanZyne marked this pull request as ready for review November 25, 2024 07:55
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

if (m_KernelMap.find(Kernel) != m_KernelMap.end()) {
return m_KernelMap[Kernel];
}
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add assertions for all caller of getKernelInfo to avoid segment fault. I see there are some guard with assertion, but some are not.

Copy link
Contributor

@yingcong-wu yingcong-wu Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think this is better than old implement(return nullptr when not found), then there will be a segv that we will not miss. But adding assertion after a call that may return nullptr is better.

Copy link
Contributor Author

@AllanZyne AllanZyne Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needn't assert, I did this on purpose.
It possible that some of kernel are not in m_KernelMap.
I have added "if" checkers when calling getKernelInfo.

You're right, I will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KernelInfo doesn't like other XXXinfo.
It's possible that some of kernel are not in m_KernelMap.

@@ -111,9 +111,9 @@ struct ProgramInfo {
ur_program_handle_t Handle;
std::atomic<int32_t> RefCount = 1;

// lock this mutex if following fields are accessed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the program is only built once, then this would not be a problem to use mutex. I don't like the "likely" in the comment unless it has proves. I prefer keeping the mutex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced with @zhaomaosu, mutex can be removed here.
I removed "likely" in comment.

@AllanZyne
Copy link
Contributor Author

Hi @oneapi-src/unified-runtime-maintain, please review, thank you.

Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just remove the unnecessary loop.

@kbenzie kbenzie added the v0.11.x Include in the v0.11.x release label Nov 25, 2024
@pbalcer pbalcer added the ready to merge Added to PR's which are ready to merge label Nov 26, 2024
@callumfare callumfare merged commit f2af85f into oneapi-src:main Nov 26, 2024
71 of 75 checks passed
@AllanZyne AllanZyne deleted the review/yang/fix_kernel_filter branch November 27, 2024 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug ready to merge Added to PR's which are ready to merge sanitizer Sanitizer layer issues/changes/specification v0.11.x Include in the v0.11.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants