[DeviceSanitizer] Fix interceptor destruction order #1879

AllanZyne · 2024-07-22T05:36:30Z

Adapters are released earlier than loader by SYCL Runtime, so we hold them in interceptor to prevent crash

pbalcer

This bit us a couple of times now. Can you please add some tests in UR to verify that the sanitizer at least initializes, parses options and destroys correctly.
Our CI infra now has PVC systems (the label is "L0_E2E"), so you could even add tests for the core functionality, but for now it would be good to cover just the general init/teardown flow.

yingcong-wu · 2024-07-22T08:33:17Z

source/loader/layers/sanitizer/ur_sanddi.cpp

+    if (result == UR_RESULT_SUCCESS) {
+        const uint32_t NumAdapters = pNumAdapters ? *pNumAdapters : NumEntries;
+        for (uint32_t i = 0; i < NumAdapters; ++i) {
+            UR_CALL(getContext()->interceptor->holdAdapter(phAdapters[i]));


If urAdapterGet() get called multiple times, then we would have duplicated handles.

maybe we needn't hold the handle of adapter, it seems ur loader has already hold this.
I'll investigate this.

every time you call urAdapterGet, an internal reference count is incremented and you need to decrement it using urAdapterRelease.

AllanZyne · 2024-07-22T08:35:18Z

This bit us a couple of times now. Can you please add some tests in UR to verify that the sanitizer at least initializes, parses options and destroys correctly. Our CI infra now has PVC systems (the label is "L0_E2E"), so you could even add tests for the core functionality, but for now it would be good to cover just the general init/teardown flow.

Sorry for the delay! I will add some tests in this PR.

AllanZyne · 2024-08-02T08:29:48Z

unified-runtime/source/loader/ur_lib.hpp

    const std::vector<LayerData> layers = {
        {ur_validation_layer::getContext(),
         ur_validation_layer::context_t::forceDelete},
#if UR_ENABLE_TRACING
        {ur_tracing_layer::getContext(),
         ur_tracing_layer::context_t::forceDelete},
#endif
#if UR_ENABLE_SANITIZER
        {ur_sanitizer_layer::getContext(),
         ur_sanitizer_layer::context_t::forceDelete},
#endif

~~Hi @pbalcer, is ur_validation_layer's context released before ur_sanitizer_layer?~~

I had found where this logic is.
The layers' releasing order needs to be reversed.

pbalcer · 2024-08-02T08:53:27Z

~~

I had found where this logic is. The layers' releasing order needs to be reversed.

Makes sense, please feel free to do that change.

AllanZyne · 2024-08-06T07:00:53Z

@pbalcer I think this fix is also needed for our next release, can you help to add a label for it?

AllanZyne · 2024-08-06T07:04:43Z

LLVM test: intel/llvm#14963

pbalcer · 2024-08-06T07:08:28Z

@AllanZyne at this point things will need to be cherry-picked into the release branch, https://github.com/oneapi-src/unified-runtime/tree/v0.10.x. No need to do that now though. Just make sure all the PRs that you want included are tagged with v0.10.x.

AllanZyne · 2024-08-07T03:22:54Z

@AllanZyne at this point things will need to be cherry-picked into the release branch, https://github.com/oneapi-src/unified-runtime/tree/v0.10.x. No need to do that now though. Just make sure all the PRs that you want included are tagged with v0.10.x.

Ok, thanks for your information.

I've checked CI tests in UR and LLVM, it seems they're unrelated to this PR.

source/loader/layers/sanitizer/asan_interceptor.cpp

yingcong-wu

LGTM other than a commenting problem.

test/layers/sanitizer/asan.cpp

zhaomaosu

lgtm

AllanZyne · 2024-08-15T02:26:42Z

Hi @oneapi-src/unified-runtime-maintain, since this PR is an essential bugfix for next release, can you help to review and merge this PR ASAP?
Thanks very much!

aarongreig

LGTM pending comments

source/loader/layers/sanitizer/ur_sanddi.cpp

test/layers/sanitizer/asan.cpp

test/layers/sanitizer/CMakeLists.txt

AllanZyne · 2024-08-19T02:32:29Z

Hi @oneapi-src/unified-runtime-maintain, should I cherry pick this PR by myself?

aarongreig · 2024-08-19T09:58:38Z

Hi @oneapi-src/unified-runtime-maintain, should I cherry pick this PR by myself?

The bump for this was included in intel/llvm#15101 which is already on our list to cherry-pick

[DeviceSanitizer] Fix interceptor destruction order

fix destruction

df5fd8b

AllanZyne requested a review from a team as a code owner July 22, 2024 05:36

github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Jul 22, 2024

AllanZyne changed the title ~~[DeviceSanitizer] Fix intercepter destruction order~~ [DeviceSanitizer] Fix interceptor destruction order Jul 22, 2024

Add UR_CALL

6449148

pbalcer reviewed Jul 22, 2024

View reviewed changes

yingcong-wu reviewed Jul 22, 2024

View reviewed changes

AllanZyne added 2 commits July 22, 2024 01:40

using unordered_set

ae7dea6

wip

9e6923f

aarongreig mentioned this pull request Jul 24, 2024

Don't retain device handle references in sanitizer layer. #1883

Merged

Merge branch 'llvm' into review/yang/fix_dsan_destruction

56ed0b8

fix crash

5859e3c

AllanZyne marked this pull request as draft August 2, 2024 08:44

AllanZyne added 2 commits August 5, 2024 19:24

fix repeat hold adapter handle

982667e

update test

d67cfec

AllanZyne marked this pull request as ready for review August 6, 2024 06:55

Merge branch 'llvm' into review/yang/fix_dsan_destruction

fbecf2a

pbalcer added the v0.10.x Include in the v0.10.x release label Aug 6, 2024

AllanZyne mentioned this pull request Aug 6, 2024

[DeviceSanitizer] Fix interceptor destruction order intel/llvm#14963

Closed

add mutex for adapter

250f759

AllanZyne added 2 commits August 6, 2024 21:48

fix test

864da64

Merge branch 'llvm' into review/yang/fix_dsan_destruction

3e3bd51

AllanZyne requested review from pbalcer and yingcong-wu August 14, 2024 02:14

zhaomaosu reviewed Aug 14, 2024

View reviewed changes

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

yingcong-wu approved these changes Aug 14, 2024

View reviewed changes

test/layers/sanitizer/asan.cpp Outdated Show resolved Hide resolved

AllanZyne added 2 commits August 14, 2024 14:37

address comments

e264cc1

fix comments

74d30dc

zhaomaosu approved these changes Aug 14, 2024

View reviewed changes

aarongreig reviewed Aug 15, 2024

View reviewed changes

source/loader/layers/sanitizer/ur_sanddi.cpp Show resolved Hide resolved

test/layers/sanitizer/asan.cpp Outdated Show resolved Hide resolved

test/layers/sanitizer/CMakeLists.txt Outdated Show resolved Hide resolved

fix reviews

fe18b4a

aarongreig approved these changes Aug 15, 2024

View reviewed changes

aarongreig added the ready to merge Added to PR's which are ready to merge label Aug 15, 2024

omarahmed1111 merged commit 1a1839b into oneapi-src:main Aug 16, 2024

omarahmed1111 mentioned this pull request Aug 16, 2024

[UR][L0] Fixed program info binary size query and fix program handle … intel/llvm#15101

Merged

kbenzie pushed a commit that referenced this pull request Aug 20, 2024

Merge pull request #1879 from AllanZyne/review/yang/fix_dsan_destruction

3ec0719

[DeviceSanitizer] Fix interceptor destruction order

kbenzie mentioned this pull request Aug 20, 2024

Candidate for the v0.10.0 release tag #1938

Merged

53 tasks

[DeviceSanitizer] Fix interceptor destruction order #1879

[DeviceSanitizer] Fix interceptor destruction order #1879

Uh oh!

Conversation

AllanZyne commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbalcer left a comment

Choose a reason for hiding this comment

Uh oh!

yingcong-wu Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

AllanZyne Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbalcer Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

AllanZyne Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

AllanZyne commented Jul 22, 2024

Uh oh!

AllanZyne commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbalcer commented Aug 2, 2024

Uh oh!

AllanZyne commented Aug 6, 2024

Uh oh!

AllanZyne commented Aug 6, 2024

Uh oh!

pbalcer commented Aug 6, 2024

Uh oh!

AllanZyne commented Aug 7, 2024

Uh oh!

Uh oh!

yingcong-wu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaomaosu left a comment

Choose a reason for hiding this comment

Uh oh!

AllanZyne commented Aug 15, 2024

Uh oh!

aarongreig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AllanZyne commented Aug 19, 2024

Uh oh!

aarongreig commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AllanZyne commented Jul 22, 2024 •

edited

Loading

AllanZyne Aug 2, 2024 •

edited

Loading

AllanZyne commented Aug 2, 2024 •

edited

Loading