Skip to content

[DeviceSanitizer] Do sanitize for device globals in AddressSanitizer pass #13678

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

Merged
merged 19 commits into from
Nov 19, 2024

Conversation

zhaomaosu
Copy link
Contributor

@zhaomaosu zhaomaosu commented May 7, 2024

Now UR already implemented API "urProgramGetGlobalVariablePointer", so we can use it to query the size of device globals and remove "__AsanDeviceGlobalCount".

UR Part: oneapi-src/unified-runtime#1584

…pass

Now UR already implemented API "urProgramGetGlobalVariablePointer",
so we can use it to query the size of device globals and remove
"__AsanDeviceGlobalCount".
Copy link
Contributor

@yingcong-wu yingcong-wu left a comment

Choose a reason for hiding this comment

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

lgtm.

@zhaomaosu zhaomaosu temporarily deployed to WindowsCILock May 7, 2024 07:35 — with GitHub Actions Inactive
@AllanZyne
Copy link
Contributor

I suggest add a flag to control this feature.

@zhaomaosu zhaomaosu temporarily deployed to WindowsCILock May 9, 2024 08:21 — with GitHub Actions Inactive
@zhaomaosu
Copy link
Contributor Author

I suggest add a flag to control this feature.

Done

@zhaomaosu zhaomaosu temporarily deployed to WindowsCILock May 9, 2024 09:04 — with GitHub Actions Inactive
@zhaomaosu zhaomaosu marked this pull request as ready for review October 29, 2024 08:39
@zhaomaosu zhaomaosu requested review from a team as code owners October 29, 2024 08:39
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm from a high level, will leave in depth review to devsan reviewers

if (SF->isIR() && (Name == "llvm.used" || Name == "llvm.compiler.used"))
// llvm.used and llvm.compiler.used and __AsanDeviceGlobalMetadata to
// the list of defined symbols.
if (SF->isIR() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

should we upstream this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a device sanitizer specific change, it's better to only keep it in intel/llvm unless we have plans to upstream whole device sanitizer implementation.

Copy link
Contributor

@wenju-he wenju-he left a comment

Choose a reason for hiding this comment

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

LGTM

@callumfare
Copy link
Contributor

@intel/llvm-gatekeepers Please merge

@sommerlukas sommerlukas merged commit 33fe64c into intel:sycl Nov 19, 2024
13 checks passed
@zhaomaosu zhaomaosu deleted the simplify-device-global branch November 21, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants