llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[DeviceSanitizer] Support detecting out-of-bounds error on DeviceGlobals

Open zhaomaosu opened this issue 1 year ago • 5 comments
trafficstars

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

zhaomaosu avatar Feb 19 '24 03:02 zhaomaosu

Hi, @zhaomaosu Does this patch support following scenario: clang++ -fsycl -fsanitize=address -c user_code1.cpp -o user1.o clang++ -fsycl -fsanitize=address -c user_code2.cpp -o user2.o clang++ -fsycl user1.o user2.o -o user.exe Both user_code1.cpp and user_code2.cpp include device globals. Thanks.

jinge90 avatar Feb 19 '24 03:02 jinge90

Hi, @zhaomaosu Does this patch support following scenario: clang++ -fsycl -fsanitize=address -c user_code1.cpp -o user1.o clang++ -fsycl -fsanitize=address -c user_code2.cpp -o user2.o clang++ -fsycl user1.o user2.o -o user.exe Both user_code1.cpp and user_code2.cpp include device globals. Thanks.

Hi, @zhaomaosu Could you add some tests to cover this scenario? Thanks.

jinge90 avatar Feb 20 '24 05:02 jinge90

Hi, @zhaomaosu Could you add some tests to cover this scenario? Thanks.

Ok, I'll add tests for it.

zhaomaosu avatar Feb 20 '24 05:02 zhaomaosu

Hi, @zhaomaosu Could you add some tests to cover this scenario? Thanks.

Done

zhaomaosu avatar Feb 20 '24 06:02 zhaomaosu

@intel/llvm-reviewers-runtime , @intel/dpcpp-tools-reviewers could you please help review this PR? Thanks.

zhaomaosu avatar Feb 22 '24 09:02 zhaomaosu

Hi @steffenlarsen, I noticed there are some failures in GEN12 tests, but I think my changes will not affect it. Do you know if these are known failures?

zhaomaosu avatar Feb 23 '24 05:02 zhaomaosu

The Gen12 Windows failures are reported in https://github.com/intel/llvm/issues/12797 and https://github.com/intel/llvm/issues/12798 so they are completely unrelated to these changes.

The Linux one I've heard was a problem in passing. I suspect it is also unrelated.

steffenlarsen avatar Feb 23 '24 06:02 steffenlarsen

Hi @steffenlarsen, the PR in UR repo has been merged. I updated the UR tag. May I get your approval?

zhaomaosu avatar Feb 27 '24 04:02 zhaomaosu

Review still required from @intel/unified-runtime-reviewers & @intel/dpcpp-tools-reviewers. Friendly ping.

steffenlarsen avatar Feb 27 '24 08:02 steffenlarsen

@maksimsab, any other comments?

zhaomaosu avatar Feb 29 '24 08:02 zhaomaosu

@maksimsab if the changes you requested are sufficient please approve the PR.

@intel/dpcpp-esimd-reviewers please review this ASAP, this PR is blocking any progress of merging other UR changes.

This is affecting multiple teams. Any help to expedite the approval and merging of this PR would be greatly appriciated.

kbenzie avatar Feb 29 '24 11:02 kbenzie

I am OK to let this PR go in order to unblock other teams. Currently, the new Pass has 0 test coverage. @zhaomaosu Are you ok to address this in the follow-up PR?

maksimsab avatar Feb 29 '24 14:02 maksimsab

I am OK to let this PR go in order to unblock other teams. Currently, the new Pass has 0 test coverage. @zhaomaosu Are you ok to address this in the follow-up PR?

Sure, I'll do this later. Thank you.

zhaomaosu avatar Feb 29 '24 14:02 zhaomaosu

Can anyone help merge this PR? Thanks a lot.

zhaomaosu avatar Feb 29 '24 15:02 zhaomaosu