llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[DeviceSanitizer] Support nullpointer detection & enable GPU tests

Open AllanZyne opened this issue 1 year ago • 4 comments

UR: https://github.com/oneapi-src/unified-runtime/pull/1914

AllanZyne avatar Aug 01 '24 05:08 AllanZyne

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

AllanZyne avatar Aug 01 '24 06:08 AllanZyne

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

AllanZyne avatar Aug 02 '24 02:08 AllanZyne

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

Recently upgraded gpu driver in https://github.com/intel/llvm/pull/14838 has support of "__devicelib_exit" builtin

wenju-he avatar Aug 02 '24 02:08 wenju-he

@intel/unified-runtime-reviewers, please review

AllanZyne avatar Aug 23 '24 01:08 AllanZyne

Kindly ping @intel/unified-runtime-reviewers

AllanZyne avatar Sep 14 '24 03:09 AllanZyne

@intel/llvm-gatekeepers please merge, Thanks!

omarahmed1111 avatar Sep 18 '24 09:09 omarahmed1111

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

steffenlarsen avatar Sep 18 '24 09:09 steffenlarsen

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

omarahmed1111 avatar Sep 19 '24 11:09 omarahmed1111

@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.

steffenlarsen avatar Sep 19 '24 18:09 steffenlarsen

@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.

yingcong-wu avatar Sep 20 '24 01:09 yingcong-wu

@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 https://github.com/intel/llvm/pull/15450.

steffenlarsen avatar Sep 20 '24 06:09 steffenlarsen

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 https://github.com/intel/llvm/pull/15450.

nsirgien avatar Sep 25 '24 16:09 nsirgien

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 https://github.com/intel/llvm/issues/15449 and https://github.com/intel/llvm/issues/15453.

steffenlarsen avatar Sep 26 '24 06:09 steffenlarsen