[SYCL][DeviceSanitizer] Checking "sycl::free" related errors #12882

AllanZyne · 2024-03-01T10:31:53Z

This PR added supports for checking the following types of error in "UR_LAYER_ASAN":

bad-free: the memory address to be freed is not allocated by UR
bad-context: the memory address to be freed uses a wrong "context"
double-free: the memory address to be freed is already freed
use-after-free: the freed memory is used in kernel

I added the environment variable "UR_LAYER_ASAN_OPTIONS" to have additional control over "UR_LAYER_ASAN", which is similar to "ASAN_OPTIONS" in ASan. Currently, it supports:

"quarantine_size_mb" (default = 0)
- Size (in MB) of quarantine per device. The pointers passed to urUSMFree are not freed immediately, but saved into QuarantineCache (per device cache), and when the cached chunk size (only counts the size of USM buffer, not shadow memory) is more than "quarantine_size_mb", the first enqueued chunk will be freed (aka., FIFO). Lower value may reduce memory usage but increase the chance of false negatives
- This option must be enabled for checking "double-free" and "use-after-free"
"debug" (default = 0)
- Print extra debug messages in kernel ("__AsanDebug” in “libdevice/sanitizer_utils.cpp”), which is helpful for DeviceSanitizer developers.

For example, to enable "use-after-free" with 5MB quarantine cache and debug message in kernel, you need to

UR_LAYER_ASAN_OPTIONS="quarantine_size_mb:5;debug:1" ./sycl_app

xtian-github · 2024-04-17T15:54:14Z

@intel/unified-runtime-reviewers who is the right person to do the merge? We need to merge this PR for Aurora deliverable asap. Thanks.

dm-vodopyanov · 2024-04-17T15:57:29Z

@intel/unified-runtime-reviewers who is the right person to do the merge? We need to merge this PR for Aurora deliverable asap. Thanks.

@xtian-github the process is described here: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#merge

We still need an approval from @intel/unified-runtime-reviewers.

sycl/test-e2e/AddressSanitizer/bad-free/bad-free-host.cpp

sycl/test-e2e/AddressSanitizer/bad-free/bad-free-minus1.cpp

aelovikov-intel · 2024-04-17T16:07:45Z

sycl/test-e2e/AddressSanitizer/common/kernel-debug.cpp

+    h.single_task<class MyKernel>([=]() { *array = 0; });
+  });
+  Q.wait();
+  // CHECK-DEBUG: [kernel]


What is the purpose of this test?

This test is used to test UR_LAYER_ASAN_OPTIONS=debug:1 runtime flag.

And what is it supposed to be, exactly? Imagine someone not familiar with sanitizer implementation reading this test, what could they find?

Are you even sure this single line should be tested at all?

Also, array leaks.

And what is it supposed to be, exactly? Imagine someone not familiar with sanitizer implementation reading this test, what could they find?

In fact, it's used to print debug message in libdevice (you can see "__AsanDebug" in "libdevice/sanitizer_utils.cpp").

Are you even sure this single line should be tested at all?

Makes sense.
Currently, UR_LAYER_ASAN_OPTIONS=debug:1 is more useful for us (sanitizer developers).
Besides, we're going to remove some of messages in release build for performance.

Okay, I'll remove this test!

aelovikov-intel · 2024-04-17T16:08:04Z

sycl/test-e2e/AddressSanitizer/common/kernel-debug.cpp

@@ -0,0 +1,17 @@
+// REQUIRES: linux, cpu
+// RUN: %{build} %device_sanitizer_flags -O2 -g -o %t
+// RUN: env SYCL_PREFER_UR=1 UR_LAYER_ASAN_OPTIONS=debug:1 %{run} %t 2>&1 | FileCheck --check-prefixes CHECK-DEBUG %s


How does it work without UR_ENABLE_LAYERS=UR_LAYER_ASAN?

Thanks for reviewing this PR.

UR_ENABLE_LAYERS=UR_LAYER_ASAN is used to forcedly enable ASan Layer in UR loader, which is the runtime support for device sanitizer.
We enable UR_LAYER_ASAN automatically by checking kernel image property.
But some tests don't have any kernel, so I have to enable UR_LAYER_ASAN manually.
Our users are unlikely to need enable device ASan without any kernel, I think it's fine to just use this flag for tests.

kbenzie · 2024-04-17T16:12:40Z

@intel/unified-runtime-reviewers who is the right person to do the merge? We need to merge this PR for Aurora deliverable asap. Thanks.

@xtian-github the process is described here: https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#merge

We still need an approval from @intel/unified-runtime-reviewers.

We need to merge oneapi-src/unified-runtime#1402 first, its next in line. Once that's in the UNIFIED_RUNTIME_TAG will need to be updated, then we'll approve and it can ben merged.

xtian-github · 2024-04-17T17:00:21Z

@cdai2 @wenju-he Would you address the review comments? Thanks.

kbenzie

UR LGTM

xtian-github

LGTM

AllanZyne · 2024-04-18T01:52:35Z

bad-free-host.cpp

I'll address them asap. Thanks!

…flags", add "force_device_asan_rt"

AllanZyne · 2024-04-18T06:10:51Z

It seems like that the CI test failure is not related to this PR.

kbenzie · 2024-04-18T14:38:50Z

@intel/llvm-gatekeepers please merge

AllanZyne · 2024-04-19T04:55:13Z

Hi @intel/llvm-gatekeepers, all comments have been addressed, please merge. Thanks!

AllanZyne and others added 30 commits December 25, 2023 23:07

init

3d7882b

fix code format

7bf11b1

clean code

13304f9

clean code

eb3ae88

clean code

f8f76de

fix comment

e58fa03

fix PVC local memory

ff8449b

clean code

fc40624

fix build

f153c32

tmp: force SYCL_PI_UR_SOURCE_DIR

10b83d9

remove asan.module_ctor

b9d0809

remove itto from libsycl-sanitizer.o

727a642

fix post link and lit

fb52e33

Merge branch 'sycl' into review/yang/sanitizer-cpu-local

1b9e96e

clean code

006a1e5

lit add more options

f423014

clean code

f51e62d

clean code

874a38e

add tests

caecf6b

Switch to use DeviceGlobal

47bd3f1

Add asan spir test

aaa1b80

change ur repo

5f9de16

add e2e test

66895de

fix format

1e32baa

Merge branch 'sycl' into review/yang/sanitizer-cpu-local

ba03cd7

follow the comments

3c921af

fix format

eb4a904

Merge branch 'sycl' into review/yang/sanitizer-cpu-local

15afd9a

update test

27545b5

install ur_adapter_opencl

ad5c2bc

AlexeySachkov approved these changes Apr 17, 2024

View reviewed changes

dm-vodopyanov changed the title ~~[DeviceSanitizer] Checking "sycl::free" related errors~~ [SYCL][DeviceSanitizer] Checking "sycl::free" related errors Apr 17, 2024

aelovikov-intel reviewed Apr 17, 2024

View reviewed changes

aarongreig added 2 commits April 17, 2024 18:01

Merge branch 'sycl' into review/yang/use-after-free

8367ad1

Update to latest UR tag + sync adapters

24535c1

aarongreig had a problem deploying to WindowsCILock April 17, 2024 17:06 — with GitHub Actions Error

Revert UR repo override.

6fe6a8f

kbenzie approved these changes Apr 17, 2024

View reviewed changes

aarongreig temporarily deployed to WindowsCILock April 17, 2024 17:09 — with GitHub Actions Inactive

xtian-github approved these changes Apr 17, 2024

View reviewed changes

aarongreig temporarily deployed to WindowsCILock April 17, 2024 17:56 — with GitHub Actions Inactive

Use local lit config change "device_sanitizer_flags" to "device_asan_…

b9c0aed

…flags", add "force_device_asan_rt"

AllanZyne requested review from a team as code owners April 18, 2024 03:06

AllanZyne had a problem deploying to WindowsCILock April 18, 2024 03:06 — with GitHub Actions Error

AllanZyne force-pushed the review/yang/use-after-free branch from fb83bfc to b9c0aed Compare April 18, 2024 03:09

AllanZyne requested a review from aelovikov-intel April 18, 2024 03:17

AllanZyne temporarily deployed to WindowsCILock April 18, 2024 03:18 — with GitHub Actions Inactive

AllanZyne temporarily deployed to WindowsCILock April 18, 2024 03:57 — with GitHub Actions Inactive

aelovikov-intel approved these changes Apr 18, 2024

View reviewed changes

steffenlarsen merged commit 4723efc into intel:sycl Apr 19, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][DeviceSanitizer] Checking "sycl::free" related errors #12882

[SYCL][DeviceSanitizer] Checking "sycl::free" related errors #12882

AllanZyne commented Mar 1, 2024 •

edited

Loading

xtian-github commented Apr 17, 2024

dm-vodopyanov commented Apr 17, 2024

aelovikov-intel Apr 17, 2024

AllanZyne Apr 18, 2024

aelovikov-intel Apr 18, 2024

aelovikov-intel Apr 18, 2024

AllanZyne Apr 19, 2024 •

edited

Loading

aelovikov-intel Apr 17, 2024

xtian-github Apr 17, 2024

AllanZyne Apr 18, 2024 •

edited

Loading

kbenzie commented Apr 17, 2024

xtian-github commented Apr 17, 2024

kbenzie left a comment

xtian-github left a comment

AllanZyne commented Apr 18, 2024

AllanZyne commented Apr 18, 2024

kbenzie commented Apr 18, 2024

AllanZyne commented Apr 19, 2024

[SYCL][DeviceSanitizer] Checking "sycl::free" related errors #12882

[SYCL][DeviceSanitizer] Checking "sycl::free" related errors #12882

Conversation

AllanZyne commented Mar 1, 2024 • edited Loading

xtian-github commented Apr 17, 2024

dm-vodopyanov commented Apr 17, 2024

aelovikov-intel Apr 17, 2024

Choose a reason for hiding this comment

AllanZyne Apr 18, 2024

Choose a reason for hiding this comment

aelovikov-intel Apr 18, 2024

Choose a reason for hiding this comment

aelovikov-intel Apr 18, 2024

Choose a reason for hiding this comment

AllanZyne Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

aelovikov-intel Apr 17, 2024

Choose a reason for hiding this comment

xtian-github Apr 17, 2024

Choose a reason for hiding this comment

AllanZyne Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

kbenzie commented Apr 17, 2024

xtian-github commented Apr 17, 2024

kbenzie left a comment

Choose a reason for hiding this comment

xtian-github left a comment

Choose a reason for hiding this comment

AllanZyne commented Apr 18, 2024

AllanZyne commented Apr 18, 2024

kbenzie commented Apr 18, 2024

AllanZyne commented Apr 19, 2024

AllanZyne commented Mar 1, 2024 •

edited

Loading

AllanZyne Apr 19, 2024 •

edited

Loading

AllanZyne Apr 18, 2024 •

edited

Loading