[UR][Layer] Add Sanitizer Layer #1074

AllanZyne · 2023-11-14T09:05:16Z

This PR added a new loader layer, named "sanitizer", which implemented the runtime part of device-side sanitizers: AddressSanitizer (UR_LAYER_ASAN), MemorySanitizer (UR_LAYER_MSAN, planned), and ThreadSanitizer (UR_LAYER_TSAN, planned).

Currently, it implemented some basic functions for PVC on the level-zero adapter, which can detect and report out-of-bounds errors on PVC.

This commit adds the adapter implementations of the virtual memory extension functionality to be used in intel/llvm#8954. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

This reverts commit 84a3afa.

This reverts commit 336ce89.

This reverts commit 9f90e43.

This reverts commit 9ae8a99.

This reverts commit 6ba94ac.

This reverts commit c36bd72.

…ase" This reverts commit 1096b0f.

This reverts commit 67fcd5e.

.gitignore

wenju-he · 2023-12-21T05:58:35Z

CMakeLists.txt

@@ -34,6 +34,7 @@ option(UR_USE_UBSAN "enable UndefinedBehaviorSanitizer" OFF)
 option(UR_USE_MSAN "enable MemorySanitizer" OFF)
 option(UR_USE_TSAN "enable ThreadSanitizer" OFF)
 option(UR_ENABLE_TRACING "enable api tracing through xpti" OFF)
+option(UR_ENABLE_SANITIZER "enable device sanitizer" ON)


the names UR_ENABLE_SANITIZER and UR_USE_ASAN might be a little confusing in the sense that former is for device only and the latter is for host only. The difference isn't shown in the names.

I follow the same naming style as the tracing layer.

I follow the same naming style as the tracing layer.

I think the confusing still exists even if it is the same naming style.

I agree. IMO, the flag to enable layers needs a new naming style. @pbalcer

hmm, UR_ENABLE_LAYER_XYZ ?

hmm, UR_ENABLE_LAYER_XYZ ?

Yes, this style is very similar to "UR_BUILD_ADAPTER_XYZ"

source/loader/layers/sanitizer/asan_interceptor.hpp

source/loader/layers/sanitizer/common.hpp

source/loader/layers/sanitizer/asan_interceptor.cpp

wenju-he · 2023-12-21T06:44:03Z

source/loader/layers/sanitizer/asan_interceptor.cpp

+ ur_event_handle_t ReadEvent{};
+
+ // If kernel has defined SPIR_DeviceSanitizerReportMem, then we try to read it
+ // to host, but it's okay that it isn't defined


In which case is SPIR_DeviceSanitizerReportMem not defined?

The below code isn't handling the case when DeviceGlobalVariableRead fails even if SPIR_DeviceSanitizerReportMem exists.

In which case is SPIR_DeviceSanitizerReportMem not defined?

When there are no memory read/write in the kernel, for example, an empty kernel.

I think a better approach is to query if SPIR_DeviceSanitizerReportMem exists and then read. This would cover all fail cases.

There's no API to query if the device global variables exist.

both OpenCL and LevelZero have API to get a pointer to a global variable. Do you mean unified runtime doesn't have this API? Is this a gap in unified runtime?
I don't think it is elegant to write to a global variable without querying if it exists.

I filed an issue to track this. #1214

source/loader/layers/sanitizer/asan_interceptor.cpp

wenju-he · 2023-12-21T06:47:23Z

source/loader/layers/sanitizer/asan_interceptor.cpp

+
+ if (IsNeedMapPhysicalMem) {
+ // We use fixed GPU PageSize: 64KB
+ const size_t PageSize = 64 * 1024u;


shouldn't the page size be queried from device?

It seems like there's no api doing this?

Can zeVirtualMemQueryPageSize used?

If there is no such API, what is the rational of choosing 64K size here?

wenju-he · 2023-12-21T06:58:51Z

source/loader/layers/sanitizer/asan_interceptor.cpp

+ur_result_t SanitizerInterceptor::addContext(ur_context_handle_t Context) {
+ auto ContextInfo = std::make_shared<ur_sanitizer_layer::ContextInfo>();
+
+ // Host Device


Is this comment accurate? SYCL doesn't have a host device.
I suppose you meant just Host?
However, the code below DeviceInfo->Type = DeviceType::CPU is showing a CPU device is initialized.

If the intent is to initialize information for the host, do we currently have logic to handle the coordination between host and a device? I suggest to to remove below code since it is for the future. When the coordination is in place, we can add below code back.

I suppose you meant just Host?

Yes. I don't think host device is a wrong term. Host code runs on the host device.

If the intent is to initialize information for the host, do we currently have logic to handle the coordination between host and a device?

Yes, we have. I have saved all host USM allocation information in the host device.

SYCL doesn't have host device. What you meant is just "host". Please don't use "host device".

Yes, we have. I have saved all host USM allocation information in the host device.

Could you please point me to the code where you saved the info?

unified-runtime/source/loader/layers/sanitizer/asan_interceptor.cpp

Lines 91 to 92 in 79998b9

// Device is nullptr if Type == USMMemoryType::HOST

auto DeviceInfo = ContextInfo->getDeviceInfo(Device);

according to offline sync, @AllanZyne would you remove this part of code?

wenju-he · 2024-01-04T07:10:07Z

source/loader/layers/sanitizer/asan_interceptor.cpp

- auto DeviceInfo = ContextInfo->getDeviceInfo(Device);
+ std::shared_ptr<DeviceInfo> DeviceInfo;
+ if (Device) {
+ DeviceInfo = ContextInfo->getDeviceInfo(Device);


nit: brace not needed for single line code according to llvm coding style. What's the coding style using in UR? Please also see other changes in this commit.

Is the 4-space indent aligning with UR coding style?

What's the coding style using in UR? Please also see other changes in this commit.

I searched some of UR codes, they add braces for single line code as well.

Is the 4-space indent aligning with UR coding style?

Yes.

wenju-he

LGTM.

source/loader/layers/sanitizer/asan_interceptor.cpp

AllanZyne · 2024-01-08T03:13:23Z

Hi, I think I've got 2 approves, is this PR ready to merge?
Thanks!

kbenzie · 2024-01-08T10:47:44Z

Hi, I think I've got 2 approves, is this PR ready to merge?

Not until it passes all of the GitHub Actions.

AllanZyne · 2024-01-09T01:41:39Z

Hi, I think I've got 2 approves, is this PR ready to merge?

Not until it passes all of the GitHub Actions.

Okay, I've noticed them, and I submitted a fix for them.

pbalcer

Your PR is now failing due to the use of getenv on Windows in a third-party component (xpti). This is a known issue, #1206 should suppress the problem.

cdai2 · 2024-01-15T07:59:01Z

Your PR is now failing due to the use of getenv on Windows in a third-party component (xpti). This is a known issue, #1206 should suppress the problem.

Is there a scheduling to fix this #1206 issue?
As this PR will not introduce other known issues, Could you please land this PR first? It blocks our following work.

kbenzie · 2024-01-16T10:39:32Z

Your PR is now failing due to the use of getenv on Windows in a third-party component (xpti). This is a known issue, #1206 should suppress the problem.

Is there a scheduling to fix this #1206 issue? As this PR will not introduce other known issues, Could you please land this PR first? It blocks our following work.

#1206 has now been merged, please update this branch with the latest changes on the main branch.

AllanZyne · 2024-01-16T10:43:18Z

Your PR is now failing due to the use of getenv on Windows in a third-party component (xpti). This is a known issue, #1206 should suppress the problem.

Is there a scheduling to fix this #1206 issue? As this PR will not introduce other known issues, Could you please land this PR first? It blocks our following work.

#1206 has not been merged, please update this branch with the latest changes on the main branch.

Thanks! Done.

AllanZyne · 2024-01-16T10:49:42Z

When will this PR be ready to merge? I'm willing to follow the process to make it land ASAP.

pbalcer · 2024-01-16T12:17:39Z

Once the build passes it should be ready to merge.
Windows CI failed again:

  ur_testing.vcxproj -> D:\a\unified-runtime\unified-runtime\build\lib\Debug\ur_testing.lib
D:\a\unified-runtime\unified-runtime\source\loader\layers\sanitizer\asan_interceptor.cpp(102,30): error C2220: the following warning is treated as an error [D:\a\unified-runtime\unified-runtime\build\source\loader\ur_loader.vcxproj]
  test-params.vcxproj -> D:\a\unified-runtime\unified-runtime\build\bin\Debug\test-params.exe
D:\a\unified-runtime\unified-runtime\source\loader\layers\sanitizer\asan_interceptor.cpp(102,30): warning C4244: 'argument': conversion from 'ur_sanitizer_layer::uptr' to 'ur_sanitizer_layer::u32', possible loss of data [D:\a\unified-runtime\unified-runtime\build\source\loader\ur_loader.vcxproj]
D:\a\unified-runtime\unified-runtime\source\loader\layers\sanitizer\asan_interceptor.cpp(422,9): warning C4244: 'argument': conversion from 'ur_sanitizer_layer::uptr' to 'ur_sanitizer_layer::u8', possible loss of data [D:\a\unified-runtime\unified-runtime\build\source\loader\ur_loader.vcxproj]

AllanZyne and others added 30 commits November 13, 2023 17:44

Add UR Sanitizer Layer

4c9fe03

UR Sanitizer: Add shadow memory alloc & memset

d6e589b

[UR][CUDA][L0][HIP] Add virtual memory adapter implementations

67fcd5e

This commit adds the adapter implementations of the virtual memory extension functionality to be used in intel/llvm#8954. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Remove duplicate CUDA UR_DEVICE_INFO_VIRTUAL_MEMORY_SUPPORT case

1096b0f

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Fix return of check error

c36bd72

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Fix common include in cuda device.hpp

6ba94ac

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Zero-initialize CUmemAccessDesc

9ae8a99

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Add missing newline

9f90e43

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Fix wrongful use of UR_ASSERT in cuda implementation

336ce89

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

an -> a

84a3afa

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

UR Sanitizer: fix bug

a8dadc1

UR Sanitizer: Add membuffer

4d63350

UR Sanitizer: Add membuffer red zone marker

25b1183

UR Sanitizer: add membuffer check

e6bc306

UR log

23fb1b3

UR change directory

cb8a761

UR refactor code

5c44b1d

UR refactor code

a377c94

UR: WIP partition membuffer

96eea7a

UR: clean code

0a72ea9

UR: clean code

8fa86dd

Revert "an -> a"

d9a51a7

This reverts commit 84a3afa.

Revert "Fix wrongful use of UR_ASSERT in cuda implementation"

730bf45

This reverts commit 336ce89.

UR: refactor

fed541a

Revert "Add missing newline"

ad2b1a7

This reverts commit 9f90e43.

Revert "Zero-initialize CUmemAccessDesc"

dec5f32

This reverts commit 9ae8a99.

Revert "Fix common include in cuda device.hpp"

ca58886

This reverts commit 6ba94ac.

Revert "Fix return of check error"

11cca13

This reverts commit c36bd72.

Revert "Remove duplicate CUDA UR_DEVICE_INFO_VIRTUAL_MEMORY_SUPPORT c…

3e6194e

…ase" This reverts commit 1096b0f.

Revert "[UR][CUDA][L0][HIP] Add virtual memory adapter implementations"

89febf0

This reverts commit 67fcd5e.

wenju-he reviewed Dec 21, 2023

View reviewed changes

AllanZyne mentioned this pull request Dec 26, 2023

[DeviceSanitizer] Support detecting out-of-bounds error on CPU Device & Static Local Memory #1210

Merged

AllanZyne added 2 commits January 1, 2024 21:58

follow the review comments

30c0bc1

fix build

86d4056

AllanZyne requested a review from wenju-he January 2, 2024 06:00

AllanZyne added 2 commits January 1, 2024 22:33

follow the review comments

49e2f4f

follow the review comments

79998b9

AllanZyne mentioned this pull request Jan 3, 2024

[SanitizerLayer] Support CPU & Local Memory AllanZyne/unified-runtime#3

Closed

fix urEnqueueKernelLaunch & remove host deviceinfo

6cc4913

wenju-he reviewed Jan 4, 2024

View reviewed changes

wenju-he approved these changes Jan 4, 2024

View reviewed changes

kbenzie reviewed Jan 4, 2024

View reviewed changes

source/loader/layers/sanitizer/asan_interceptor.cpp Outdated Show resolved Hide resolved

fix build

577ddd3

AllanZyne requested a review from kbenzie January 5, 2024 01:56

Merge branch 'main' into sanitizer-pr

4ee0e10

fix build

dc127d5

pbalcer approved these changes Jan 9, 2024

View reviewed changes

Merge branch 'main' into sanitizer-pr

b59d931

fix build error

28315db

pbalcer merged commit b51dec0 into oneapi-src:main Jan 17, 2024
51 checks passed

AllanZyne deleted the sanitizer-pr branch April 3, 2024 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UR][Layer] Add Sanitizer Layer #1074

[UR][Layer] Add Sanitizer Layer #1074

AllanZyne commented Nov 14, 2023 •

edited

Loading

wenju-he Dec 21, 2023

AllanZyne Jan 2, 2024

wenju-he Jan 2, 2024 •

edited

Loading

AllanZyne Jan 2, 2024 •

edited

Loading

pbalcer Jan 9, 2024

AllanZyne Jan 9, 2024

wenju-he Dec 21, 2023

AllanZyne Dec 21, 2023 •

edited

Loading

wenju-he Dec 21, 2023

AllanZyne Jan 2, 2024

wenju-he Jan 2, 2024

AllanZyne Jan 2, 2024 •

edited

Loading

wenju-he Dec 21, 2023

AllanZyne Jan 2, 2024 •

edited

Loading

wenju-he Jan 2, 2024

AllanZyne Jan 2, 2024

wenju-he Dec 21, 2023

AllanZyne Jan 2, 2024

wenju-he Jan 2, 2024

AllanZyne Jan 2, 2024

wenju-he Jan 4, 2024

AllanZyne Jan 4, 2024

wenju-he Jan 4, 2024 •

edited

Loading

AllanZyne Jan 4, 2024

wenju-he left a comment

AllanZyne commented Jan 8, 2024

kbenzie commented Jan 8, 2024

AllanZyne commented Jan 9, 2024

pbalcer left a comment •

edited

Loading

cdai2 commented Jan 15, 2024

kbenzie commented Jan 16, 2024 •

edited

Loading

AllanZyne commented Jan 16, 2024

AllanZyne commented Jan 16, 2024

pbalcer commented Jan 16, 2024 •

edited

Loading

	// Device is nullptr if Type == USMMemoryType::HOST
	auto DeviceInfo = ContextInfo->getDeviceInfo(Device);

[UR][Layer] Add Sanitizer Layer #1074

[UR][Layer] Add Sanitizer Layer #1074

Conversation

AllanZyne commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenju-he Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

AllanZyne Jan 2, 2024 • edited Loading

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

AllanZyne Dec 21, 2023 • edited Loading

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

AllanZyne Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllanZyne Jan 2, 2024 • edited Loading

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

wenju-he Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenju-he left a comment

Choose a reason for hiding this comment

AllanZyne commented Jan 8, 2024

kbenzie commented Jan 8, 2024

AllanZyne commented Jan 9, 2024

pbalcer left a comment • edited Loading

Choose a reason for hiding this comment

cdai2 commented Jan 15, 2024

kbenzie commented Jan 16, 2024 • edited Loading

AllanZyne commented Jan 16, 2024

AllanZyne commented Jan 16, 2024

pbalcer commented Jan 16, 2024 • edited Loading

AllanZyne commented Nov 14, 2023 •

edited

Loading

wenju-he Jan 2, 2024 •

edited

Loading

AllanZyne Jan 2, 2024 •

edited

Loading

AllanZyne Dec 21, 2023 •

edited

Loading

AllanZyne Jan 2, 2024 •

edited

Loading

AllanZyne Jan 2, 2024 •

edited

Loading

wenju-he Jan 4, 2024 •

edited

Loading

pbalcer left a comment •

edited

Loading

kbenzie commented Jan 16, 2024 •

edited

Loading

pbalcer commented Jan 16, 2024 •

edited

Loading