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.

Sign up for GitHub

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

Jump to bottom

[DeviceSanitizer] Support check invalid kernel argument #14512

Merged
merged 16 commits into from
Aug 8, 2024
Merged

[DeviceSanitizer] Support check invalid kernel argument #14512

merged 16 commits into from
Aug 8, 2024

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Jul 10, 2024 •
edited
Loading

UR: oneapi-src/unified-runtime#1843

also fix parallel_for_double.cpp

@AllanZyne AllanZyne requested a review from a team as a code owner July 10, 2024 08:29
@AllanZyne AllanZyne requested a review from a team as a code owner July 10, 2024 08:36
@AllanZyne AllanZyne mentioned this pull request Jul 10, 2024
Merged
yingcong-wu
yingcong-wu reviewed Jul 10, 2024
View reviewed changes
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.

out-of-bounds and released-pointer checks seem a bit redundant. I mean those could also be caught when those errors happened within kernel execution, right?

@AllanZyne
Copy link
Contributor Author

out-of-bounds and released-pointer checks seem a bit redundant. I mean those could also be caught when those errors happened within kernel execution, right?

Yeah, that's a problem. I'll reconsider this.

@AllanZyne AllanZyne marked this pull request as draft July 10, 2024 09:26
@AllanZyne
Copy link
Contributor Author

out-of-bounds and released-pointer checks seem a bit redundant. I mean those could also be caught when those errors happened within kernel execution, right?

Yes, this a bit redundant. But it costs very little to check these two errors while checking other errors.

@AllanZyne AllanZyne marked this pull request as ready for review July 16, 2024 13:16
@AllanZyne
Copy link
Contributor Author

Hi @intel/unified-runtime-reviewers, please review, thanks

AllanZyne
AllanZyne commented Jul 25, 2024
View reviewed changes
sycl/test-e2e/AddressSanitizer/out-of-bounds/USM/parallel_for_double.cpp Outdated
@@ -16,7 +16,7 @@

int main() {
sycl::queue Q;
constexpr std::size_t N = 123456;
constexpr std::size_t N = 512;
Copy link
Contributor Author

@AllanZyne AllanZyne Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NOTE: We'll add a new test in “private” for large workgroup case

@omarahmed1111
Copy link
Contributor

@intel/dpcpp-sanitizers-review could you have another round of review on this as I merged the UR part of it, so we would prioritize merging that. Thanks!

@AllanZyne
Copy link
Contributor Author

@intel/dpcpp-sanitizers-review could you have another round of review on this as I merged the UR part of it, so we would prioritize merging that. Thanks!

Sure.

@AllanZyne
Copy link
Contributor Author

@intel/llvm-gatekeepers please merge, thanks.

@steffenlarsen steffenlarsen merged commit 4093207 into sycl Aug 8, 2024
13 checks passed
@AllanZyne AllanZyne deleted the review/yang/invalid_arguments branch August 27, 2024 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yingcong-wu yingcong-wu yingcong-wu approved these changes

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@omarahmed1111 omarahmed1111 omarahmed1111 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AllanZyne @omarahmed1111 @zhaomaosu @yingcong-wu @steffenlarsen