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

zhaomaosu · 2024-02-19T03:18:07Z

UR part: oneapi-src/unified-runtime#1354

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

AllanZyne

LGTM.

Please modify UNIFIED_RUNTIME_REPO and UNIFIED_RUNTIME_TAG in "llvm/sycl/plugins/unified_runtime/CMakeLists.txt" to enable integration test.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

jinge90 · 2024-02-19T03:53:04Z

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.

*Remove attribute of device globals in libdevice

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

jinge90 · 2024-02-20T05:27:29Z

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.

zhaomaosu · 2024-02-20T05:37:07Z

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

Ok, I'll add tests for it.

zhaomaosu · 2024-02-20T06:21:02Z

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

Done

llvm/tools/sycl-post-link/sycl-post-link.cpp

zhaomaosu · 2024-02-27T04:32:34Z

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

steffenlarsen

LGTM!

steffenlarsen · 2024-02-27T08:02:01Z

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

AlexeySachkov · 2024-02-27T11:08:19Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  SmallVector<Constant *, 8> DeviceGlobalMetadata;
+
+  constexpr uint64_t MaxRZ = 1 << 18;
+  const uint64_t MinRZ = 32;


Suggested change

const uint64_t MinRZ = 32;

constexpr uint64_t MinRZ = 32;

Nit, for uniformity

AlexeySachkov · 2024-02-27T11:08:44Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  const uint64_t MinRZ = 32;
+
+  Type *IntptrTy =
+      Type::getIntNTy(M.getContext(), M.getDataLayout().getPointerSizeInBits());


Suggested change

Type::getIntNTy(M.getContext(), M.getDataLayout().getPointerSizeInBits());

Type::getIntNTy(M.getContext(), DL.getPointerSizeInBits());

AlexeySachkov · 2024-02-27T11:09:43Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+  constexpr uint64_t MaxRZ = 1 << 18;
+  const uint64_t MinRZ = 32;
+
+  Type *IntptrTy =


Suggested change

Type *IntptrTy =

Type *IntTy =

Or maybe SizeTTy. That type is not a pointer, so ptr is definitely confusing here

Changed to IntTy

AlexeySachkov · 2024-02-27T11:10:28Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+    // Non image scope device globals are implemented by device USM, and the
+    // out-of-bounds check for them will be done by sanitizer USM part. So we
+    // exclude them here.
+    if (isDeviceGlobalVariable(G) && hasDeviceImageScopeProperty(G)) {


This should be turned into an early exit instead

AlexeySachkov · 2024-02-27T11:11:00Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+
+        assert((RZ + SizeInBytes) % MinRZ == 0);
+        return RZ;
+      }();


Formatting seems to be broken

Re-run clang-format

AlexeySachkov · 2024-02-27T11:13:23Z

llvm/tools/sycl-post-link/sycl-post-link.cpp

+        } else {
+          // Calculate RZ, where MinRZ <= RZ <= MaxRZ, and RZ ~ 1/4 *
+          // SizeInBytes.
+          RZ = std::clamp((SizeInBytes / MinRZ / 4) * MinRZ, MinRZ, MaxRZ);


Am I right that (SizeInBytes / MinRZ / 4) * MinRZ is written this way on purpose, to ensure certain rounding during calculation? If so, a comment would be welcome about that.

This code is coming from https://github.com/intel/llvm/blob/sycl/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L2767.
I traced back the git histories. It seems that this code initially started out like this. So, I also don't know if it's written this way for some reason. Sorry for this.
But we do this instrumentation for device global in sycl-post-link tool temporarily. When unified runtime implemented the API urProgramGetGlobalVariablePointer, I'll move this instrumentation code back to AddressSanitizer pass.

llvm/tools/sycl-post-link/sycl-post-link.cpp

zhaomaosu · 2024-02-29T08:55:10Z

@maksimsab, any other comments?

kbenzie · 2024-02-29T11:10:34Z

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

maksimsab · 2024-02-29T14:37:01Z

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

LGTM

zhaomaosu · 2024-02-29T14:59:02Z

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 · 2024-02-29T15:03:24Z

Can anyone help merge this PR? Thanks a lot.

sarnex

esimd lgtm, leaving tools for others

Tests HIP Support for Graph from oneapi-src/unified-runtime#1254 and updates documentation. Depends on #12753. --------- Co-authored-by: Andrey Alekseenko <andrey.alekseenko@scilifelab.se> Co-authored-by: Ewan Crawford <ewan@codeplay.com>

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

626c121

zhaomaosu requested review from a team as code owners February 19, 2024 03:18

zhaomaosu requested review from cperkinsintel and AllanZyne and removed request for cperkinsintel February 19, 2024 03:18

zhaomaosu temporarily deployed to WindowsCILock February 19, 2024 03:18 — with GitHub Actions Inactive

zhaomaosu requested review from jinge90, wenju-he and cdai2 February 19, 2024 03:18

AllanZyne reviewed Feb 19, 2024

View reviewed changes

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp Outdated Show resolved Hide resolved

AllanZyne approved these changes Feb 19, 2024

View reviewed changes

wenju-he reviewed Feb 19, 2024

View reviewed changes

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp Outdated Show resolved Hide resolved

zhaomaosu temporarily deployed to WindowsCILock February 19, 2024 03:55 — with GitHub Actions Inactive

zhaomaosu marked this pull request as draft February 19, 2024 05:48

*Instrument device globals in sycl-post-link tool

b92326d

*Remove attribute of device globals in libdevice

zhaomaosu temporarily deployed to WindowsCILock February 19, 2024 08:47 — with GitHub Actions Inactive

zhaomaosu temporarily deployed to WindowsCILock February 19, 2024 09:23 — with GitHub Actions Inactive

wenju-he reviewed Feb 20, 2024

View reviewed changes

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp Outdated Show resolved Hide resolved

Add a test which has multi device images

64825c9

zhaomaosu temporarily deployed to WindowsCILock February 20, 2024 06:20 — with GitHub Actions Inactive

zhaomaosu temporarily deployed to WindowsCILock February 20, 2024 06:57 — with GitHub Actions Inactive

Add comments

f1a379c

zhaomaosu temporarily deployed to WindowsCILock February 21, 2024 03:16 — with GitHub Actions Inactive

AllanZyne reviewed Feb 21, 2024

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

zhaomaosu temporarily deployed to WindowsCILock February 21, 2024 03:53 — with GitHub Actions Inactive

zhaomaosu temporarily deployed to WindowsCILock February 27, 2024 01:50 — with GitHub Actions Inactive

zhaomaosu temporarily deployed to WindowsCILock February 27, 2024 02:27 — with GitHub Actions Inactive

zhaomaosu requested a review from steffenlarsen February 27, 2024 04:31

steffenlarsen approved these changes Feb 27, 2024

View reviewed changes

aarongreig approved these changes Feb 27, 2024

View reviewed changes

AlexeySachkov reviewed Feb 27, 2024

View reviewed changes

kbenzie mentioned this pull request Feb 27, 2024

[SYCL][Graph][HIP] Add HIP backend support to SYCL-Graph #12230

Merged

Update as per comments

9352cdd

zhaomaosu temporarily deployed to WindowsCILock February 27, 2024 12:17 — with GitHub Actions Inactive

zhaomaosu requested a review from AlexeySachkov February 27, 2024 12:22

zhaomaosu temporarily deployed to WindowsCILock February 27, 2024 12:56 — with GitHub Actions Inactive

maksimsab requested changes Feb 27, 2024

View reviewed changes

llvm/tools/sycl-post-link/sycl-post-link.cpp Outdated Show resolved Hide resolved

Add a new pass SanitizeDeviceGlobal

201f92a

zhaomaosu requested a review from a team as a code owner February 28, 2024 02:59

zhaomaosu requested a review from maksimsab February 28, 2024 03:00

zhaomaosu temporarily deployed to WindowsCILock February 28, 2024 03:03 — with GitHub Actions Inactive

zhaomaosu temporarily deployed to WindowsCILock February 28, 2024 03:40 — with GitHub Actions Inactive

maksimsab approved these changes Feb 29, 2024

View reviewed changes

sarnex approved these changes Feb 29, 2024

View reviewed changes

steffenlarsen merged commit a14cfdd into intel:sycl Feb 29, 2024
12 checks passed

zhaomaosu deleted the sanitizer-device-global branch March 1, 2024 03:37

zhaomaosu mentioned this pull request Mar 1, 2024

[SYCL][DeviceSanitizer] Add tests for sycl-post-link to cover device global #12879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zhaomaosu commented Feb 19, 2024 •

edited

Loading

AllanZyne left a comment

jinge90 commented Feb 19, 2024

jinge90 commented Feb 20, 2024

zhaomaosu commented Feb 20, 2024

zhaomaosu commented Feb 20, 2024

zhaomaosu commented Feb 27, 2024

steffenlarsen left a comment

steffenlarsen commented Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

AlexeySachkov Feb 27, 2024

zhaomaosu Feb 27, 2024

zhaomaosu commented Feb 29, 2024

kbenzie commented Feb 29, 2024 •

edited

Loading

maksimsab commented Feb 29, 2024

maksimsab left a comment

zhaomaosu commented Feb 29, 2024

zhaomaosu commented Feb 29, 2024

sarnex left a comment

	Type::getIntNTy(M.getContext(), M.getDataLayout().getPointerSizeInBits());
	Type::getIntNTy(M.getContext(), DL.getPointerSizeInBits());

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

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

Conversation

zhaomaosu commented Feb 19, 2024 • edited Loading

AllanZyne left a comment

Choose a reason for hiding this comment

jinge90 commented Feb 19, 2024

jinge90 commented Feb 20, 2024

zhaomaosu commented Feb 20, 2024

zhaomaosu commented Feb 20, 2024

zhaomaosu commented Feb 27, 2024

steffenlarsen left a comment

Choose a reason for hiding this comment

steffenlarsen commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaomaosu commented Feb 29, 2024

kbenzie commented Feb 29, 2024 • edited Loading

maksimsab commented Feb 29, 2024

maksimsab left a comment

Choose a reason for hiding this comment

zhaomaosu commented Feb 29, 2024

zhaomaosu commented Feb 29, 2024

sarnex left a comment

Choose a reason for hiding this comment

zhaomaosu commented Feb 19, 2024 •

edited

Loading

kbenzie commented Feb 29, 2024 •

edited

Loading