Skip to content

[DeviceMSAN] Support MemorySanitizer for device offloading #2325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2024
Merged

[DeviceMSAN] Support MemorySanitizer for device offloading #2325

merged 1 commit into from
Dec 11, 2024

Conversation

@AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Nov 14, 2024 •
edited
Loading

@AllanZyne AllanZyne mentioned this pull request Nov 14, 2024
Merged
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Nov 14, 2024
@AllanZyne AllanZyne marked this pull request as ready for review November 25, 2024 09:21
@AllanZyne AllanZyne requested a review from a team as a code owner November 25, 2024 09:21
@AllanZyne AllanZyne changed the title [DeviceMSAN] MemorySanitizer POC [DeviceMSAN] Support MemorySanitizer for device offloading Nov 25, 2024
@kbenzie kbenzie added the v0.11.x Include in the v0.11.x release label Nov 25, 2024
zhaomaosu
zhaomaosu reviewed Nov 26, 2024
View reviewed changes
yingcong-wu
yingcong-wu reviewed Nov 26, 2024
View reviewed changes
zhaomaosu
zhaomaosu approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Nov 28, 2024

Hi @oneapi-src/unified-runtime-maintain, internal review has done. Please review, thank you.

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Nov 28, 2024 •
edited
Loading

CI "Build and test / E2E OpenCL / Start e2e job / Build SYCL, UR, run E2E" failed is due to opencl driver haven't upgraded to 2025.0.
CI "Build and test / E2E L0 / Start e2e job / Build SYCL, UR, run E2E" is unrelated to this PR.

@cdai2
Copy link

cdai2 commented Dec 3, 2024

ping @oneapi-src/unified-runtime-maintain

@kbenzie
Copy link
Contributor

kbenzie commented Dec 3, 2024

ping @oneapi-src/unified-runtime-maintain

intel/llvm#15955 is Waiting on code owner review from intel/dpcpp-clang-driver-reviewers, intel/dpcpp-esimd-reviewers, intel/dpcpp-tools-reviewers, intel/llvm-reviewers-runtime until those code owners approve that PR this won't be merged.

@pbalcer pbalcer self-requested a review December 5, 2024 17:10
@AllanZyne
Copy link
Contributor Author

AllanZyne commented Dec 10, 2024 •
edited
Loading

Hi @oneapi-src/unified-runtime-maintain @pbalcer, I have got almost all reviewers' approve from llvm, could you please review this PR as well?
Thank you!

pbalcer
pbalcer reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@pbalcer pbalcer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly lgtm. My biggest concern is the large amounts of copy/pasting that the new msan is doing from the existing asan codebase. I understand the urgency for now, so we can move with the current implementation, but please consider how you could reduce the number of duplications in your codebase this is creating so that asan/msan are easier to maintain long-term.

source/loader/layers/sanitizer/msan/msan_interceptor.cpp
return UR_RESULT_SUCCESS;
}

ur_result_t MsanInterceptor::registerSpirKernels(ur_program_handle_t Program) {
Copy link
Contributor

@pbalcer pbalcer Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, and a few other functions in this file, are nearly straight copy-paste from asan interceptor.

I know there's time pressure now, but please create a high-priority ticket to refactor this post merge. Maybe there should be a common base class for sanitizers that contains the shared code?

Copy link
Contributor Author

@AllanZyne AllanZyne Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we noticed this problem as well, after intense period of feature developing, we will try to merge common codes to make it more easily maintain.

source/loader/layers/sanitizer/msan/msan_options.cpp
namespace ur_sanitizer_layer {
namespace msan {

MsanOptions::MsanOptions() {
Copy link
Contributor

@pbalcer pbalcer Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a near 1:1 copy/paste from asan_options.

Copy link
Contributor Author

@AllanZyne AllanZyne Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We consider to use a single sanitizer option in the future

@pbalcer pbalcer added the ready to merge Added to PR's which are ready to merge label Dec 10, 2024
@AllanZyne
Copy link
Contributor Author

AllanZyne commented Dec 10, 2024

Mostly lgtm. My biggest concern is the large amounts of copy/pasting that the new msan is doing from the existing asan codebase. I understand the urgency for now, so we can move with the current implementation, but please consider how you could reduce the number of duplications in your codebase this is creating so that asan/msan are easier to maintain long-term.

Thank you for your understanding, @pbalcer!

@kbenzie
Copy link
Contributor

kbenzie commented Dec 10, 2024

Once the L0 jobs complete I'll squash the commits then merge this.

@kbenzie
Copy link
Contributor

kbenzie commented Dec 10, 2024

After merging #2420 (since the CI was ready) there is a conflict on source/loader/layers/sanitizer/asan/asan_ddi.cpp please fix @AllanZyne and I'll merge this first thing in the morning UK time.

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Dec 11, 2024

After merging #2420 (since the CI was ready) there is a conflict on source/loader/layers/sanitizer/asan/asan_ddi.cpp please fix @AllanZyne and I'll merge this first thing in the morning UK time.

Hi @kbenzie, I have to merge main branch to solve the conflict, which is not the commit id pointed by LLVM.
Hope the CI in LLVM works well.

@kbenzie kbenzie force-pushed the review/yang/restructure_asan_msan branch from a064875 to 064da15 Compare December 11, 2024 11:04
@kbenzie kbenzie merged commit 8818ab5 into oneapi-src:main Dec 11, 2024
12 checks passed
martygrant pushed a commit to intel/llvm that referenced this pull request Dec 11, 2024
@AllanZyne @zhaomaosu @kbenzie
[DeviceMSAN] Support MemorySanitizer for device offloading (#15955)
cbd6b7c 
UR: oneapi-src/unified-runtime#2325

---------

Co-authored-by: Zhao, Maosu <[email protected]>
Co-authored-by: Kenneth Benzie (Benie) <[email protected]>
@AllanZyne AllanZyne deleted the review/yang/restructure_asan_msan branch December 12, 2024 01:11
KornevNikita pushed a commit to intel/llvm that referenced this pull request Feb 25, 2025
@AllanZyne @zhaomaosu
@kbenzie @KornevNikita
[DeviceMSAN] Support MemorySanitizer for device offloading (#15955)
9b2b854 
UR: oneapi-src/unified-runtime#2325

---------

Co-authored-by: Zhao, Maosu <[email protected]>
Co-authored-by: Kenneth Benzie (Benie) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pbalcer pbalcer pbalcer approved these changes

+2 more reviewers

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@yingcong-wu yingcong-wu yingcong-wu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

loader Loader related feature/bug ready to merge Added to PR's which are ready to merge sanitizer Sanitizer layer issues/changes/specification v0.11.x Include in the v0.11.x release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AllanZyne @cdai2 @kbenzie @pbalcer @zhaomaosu @yingcong-wu