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 nullpointer detection & enable GPU tests #14891

Merged
merged 15 commits into from
Sep 19, 2024
Merged

[DeviceSanitizer] Support nullpointer detection & enable GPU tests #14891

merged 15 commits into from
Sep 19, 2024

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Aug 1, 2024 •
edited
Loading

@AllanZyne AllanZyne requested a review from a team as a code owner August 1, 2024 05:49
@AllanZyne AllanZyne mentioned this pull request Aug 1, 2024
Merged
@AllanZyne AllanZyne requested a review from a team as a code owner August 1, 2024 05:52
@AllanZyne
Copy link
Contributor Author

Maybe we need to wait for cpu&gpu driver upgrading to support "__devicelib_exit" builtin.

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Aug 2, 2024 •
edited
Loading

gpu e2e are skiped, can't find cpu e2e results

@wenju-he
Copy link
Contributor

wenju-he commented Aug 2, 2024

Maybe we need to wait for cpu&gpu driver upgrading to support "__devicelib_exit" builtin.

Recently upgraded gpu driver in #14838 has support of "__devicelib_exit" builtin

@AllanZyne AllanZyne changed the title [DeviceSanitizer] Support nullpointer detection [DeviceSanitizer] Support nullpointer detection & enable GPU tests Aug 22, 2024
@AllanZyne
Copy link
Contributor Author

@intel/unified-runtime-reviewers, please review

@AllanZyne
Copy link
Contributor Author

Kindly ping @intel/unified-runtime-reviewers

@omarahmed1111
Copy link
Contributor

@intel/llvm-gatekeepers please merge, Thanks!

@steffenlarsen
Copy link
Contributor

Approval still needed from @intel/dpcpp-sanitizers-review (e.g. @yingcong-wu )

@omarahmed1111
Copy link
Contributor

@intel/llvm-gatekeepers This should be ready now. Please merge.

@martygrant martygrant merged commit 0985116 into sycl Sep 19, 2024
12 checks passed
@steffenlarsen
Copy link
Contributor

@AllanZyne - The following tests are failing in post-commit after this:
SYCL :: AddressSanitizer/invalid-argument/out-of-bounds.cpp
SYCL :: AddressSanitizer/invalid-argument/released-pointer.cpp
SYCL :: AddressSanitizer/nullpointer/global_nullptr.cpp
Please address these ASAP or we will will have to revert the patch.

@yingcong-wu
Copy link
Contributor

@AllanZyne - The following tests are failing in post-commit after this: SYCL :: AddressSanitizer/invalid-argument/out-of-bounds.cpp SYCL :: AddressSanitizer/invalid-argument/released-pointer.cpp SYCL :: AddressSanitizer/nullpointer/global_nullptr.cpp Please address these ASAP or we will will have to revert the patch.

Hi @steffenlarsen , @AllanZyne is OOO for now, so I think you should revert the patch.

@steffenlarsen steffenlarsen mentioned this pull request Sep 20, 2024
Closed
steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Sep 20, 2024
@steffenlarsen
[SYCL][E2E] Disable and fix address sanitizer tests for DG2
5ea0351 
This commit disables the AddressSanitizer/invalid-argument/out-of-bounds.cpp
and sycl/test-e2e/AddressSanitizer/invalid-argument/released-pointer.cpp
tests for DG2, following the enabling of address sanitizer tests for DG2
in intel#14891.

Additionally, it fixes the compilation failure in
sycl/test-e2e/AddressSanitizer/nullpointer/global_nullptr.cpp which
should hopefully allow it to pass as expected.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen mentioned this pull request Sep 20, 2024
Merged
@steffenlarsen
Copy link
Contributor

@yingcong-wu - Thank you for making me aware! Looking at the changes, I don't think we need a full revert. If I'm reading it correctly, it seems like we didn't enable DG2 testing before, so I think it would be better to just disable the failing tests. Note that global_nullptr.cpp just seems to be failing to compile, likely due to unlucky timing, so I aim to fix that too. Patch in #15450.

steffenlarsen added a commit that referenced this pull request Sep 20, 2024
@steffenlarsen
[SYCL][E2E] Disable and fix address sanitizer tests for DG2 (#15450)
2c9d980 
This commit disables the
AddressSanitizer/invalid-argument/out-of-bounds.cpp and
sycl/test-e2e/AddressSanitizer/invalid-argument/released-pointer.cpp
tests for DG2, following the enabling of address sanitizer tests for DG2
in #14891.

Additionally, it fixes the compilation failure in
sycl/test-e2e/AddressSanitizer/nullpointer/global_nullptr.cpp which
should hopefully allow it to pass as expected.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@AllanZyne AllanZyne mentioned this pull request Sep 23, 2024
Open
@AllanZyne AllanZyne deleted the review/yang/dsan_nullpointer branch September 23, 2024 02:15
@nsirgien
Copy link

nsirgien commented Sep 25, 2024 •
edited by steffenlarsen
Loading

Thank you for making me aware! Looking at the changes, I don't think we need a full revert. If I'm reading it correctly, it seems like we didn't enable DG2 testing before, so I think it would be better to just disable the failing tests.

@steffenlarsen, as far as I remember our discussion in SYCL CI meetings - it was an conclusion and agreement what we won't disable failing tests without at least tracker being created in regards of this failures.

Was it done in this case? I maybe wrong, but I don't see any tracker being mention in this PR in regards of this disabled tests and I also do not see it in #15450.

@steffenlarsen
Copy link
Contributor

Thank you for making me aware! Looking at the changes, I don't think we need a full revert. If I'm reading it correctly, it seems like we didn't enable DG2 testing before, so I think it would be better to just disable the failing tests.

@steffenlarsen, as far as I remember our discussion in SYCL CI meetings - it was an conclusion and agreement what we won't disable failing tests without at least tracker being created in regards of this failures.

Was it done in this case? I maybe wrong, but I don't see any tracker being mention in this PR in regards of this disabled tests and I also do not see it in #15450.

Hi @nsirgien! You're right, I did not mention the trackers in the PR itself. However, they are mentioned in the test files above the line disabling them. The trackers are #15449 and #15453.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omarahmed1111 omarahmed1111 omarahmed1111 approved these changes

@yingcong-wu yingcong-wu yingcong-wu 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.
7 participants
@AllanZyne @wenju-he @omarahmed1111 @steffenlarsen @yingcong-wu @nsirgien @martygrant