Skip to content
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

[ASAN] Intercept urProgramLink in sanitizer layer #1452

Merged
merged 8 commits into from
Apr 30, 2024
Merged

[ASAN] Intercept urProgramLink in sanitizer layer #1452

merged 8 commits into from
Apr 30, 2024

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Mar 18, 2024 •
edited
Loading

Openmp offloading runtime (libomptarget) calls urProgramLink to build and link user's device code, this API needs to be intercepted to register device globals.
intel/llvm: intel/llvm#13048

@jinge90 jinge90 requested a review from a team as a code owner March 18, 2024 03:45
@jinge90 jinge90 requested a review from zhaomaosu March 18, 2024 03:45
@jinge90
Copy link
Contributor Author

jinge90 commented Mar 18, 2024

Hi, @zhaomaosu
could you help review this patch?
Thanks very much.

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 12.42%. Comparing base (78ef1ca) to head (16562ed).
Report is 188 commits behind head on main.

Files Patch % Lines
source/loader/layers/sanitizer/ur_sanddi.cpp 0.00% 30 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1452      +/-   ##
==========================================
- Coverage   14.82%   12.42%   -2.41%     
==========================================
  Files         250      241       -9     
  Lines       36220    36272      +52     
  Branches     4094     4115      +21     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31762     +962     
+ Partials       51        4      -47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jinge90
[ASAN] Intercept urProgramLink to enable asan layer for openmp offloa…
ca67394 
…ding

Signed-off-by: jinge90 <ge.jin@intel.com>
@jinge90
Intercept urProgramLinkExp
6c5d277 
Signed-off-by: jinge90 <ge.jin@intel.com>
@jinge90
init program proc exp table
5eee9e3 
Signed-off-by: jinge90 <ge.jin@intel.com>
zhaomaosu
zhaomaosu approved these changes Mar 18, 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

@jinge90
Copy link
Contributor Author

jinge90 commented Mar 18, 2024

Hi, @oneapi-src/unified-runtime-maintain
Could you help review this patch?
Thanks very much.

pbalcer
pbalcer approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

lgtm, but please separate out the unrelated changes.

source/loader/layers/sanitizer/asan_interceptor.cpp Outdated Show resolved Hide resolved
@pbalcer pbalcer added the ready to merge Added to PR's which are ready to merge label Mar 18, 2024
@jinge90 jinge90 requested a review from pbalcer March 18, 2024 08:04
@pbalcer
Copy link
Contributor

pbalcer commented Mar 18, 2024

It's very unlikely that this will affect anything on intel/llvm, right? Despite that, please follow the process outlined here: https://oneapi-src.github.io/unified-runtime/core/CONTRIB.html#adapter-change-process and create an intel/llvm PR.

@jinge90
Make bug fix a separate commit
6213202 
Signed-off-by: jinge90 <ge.jin@intel.com>
@jinge90 jinge90 mentioned this pull request Mar 18, 2024
Merged
@jinge90
Copy link
Contributor Author

jinge90 commented Mar 19, 2024

Hi, @pbalcer
intel/llvm pre-ci has passed: intel/llvm#13048 , could you help merge this ur patch?
Thanks very much.

AllanZyne
AllanZyne approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@AllanZyne AllanZyne left a comment

Choose a reason for hiding this comment

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

LGTM

@kbenzie
Copy link
Contributor

kbenzie commented Mar 19, 2024 •
edited
Loading

Hi, @pbalcer
intel/llvm pre-ci has passed: intel/llvm#13048 , could you help merge this ur patch?
Thanks very much.

We need to make sure about not conflicting with the other PR's in our merge queue.

@jinge90
Copy link
Contributor Author

jinge90 commented Apr 2, 2024

Hi, @pbalcer
intel/llvm pre-ci has passed: intel/llvm#13048 , could you help merge this ur patch?
Thanks very much.

We need to make sure about not conflicting with the other PR's in our merge queue.

Hi, @kbenzie
May I know the status about this PR? Is there anything I need to do to merge the PR?
Thanks very much.

@kbenzie
Copy link
Contributor

kbenzie commented Apr 3, 2024

Hi, @kbenzie
May I know the status about this PR? Is there anything I need to do to merge the PR?
Thanks very much.

I think its in a good state, we've still got a few v0.9.x PR's to merge but I hope to be done with those soon.

@kbenzie kbenzie added the loader Loader related feature/bug label Apr 3, 2024
@kbenzie kbenzie added the sanitizer Sanitizer layer issues/changes/specification label Apr 17, 2024
@jinge90
Copy link
Contributor Author

jinge90 commented Apr 23, 2024

Hi, @pbalcer and @kbenzie
I just updated the patch to align with latest main branch, the related intel/llvm pr is updated as well, could you help review again?
Thanks very much!

@kbenzie kbenzie merged commit 1906ce7 into oneapi-src:main Apr 30, 2024
51 checks passed
martygrant pushed a commit to intel/llvm that referenced this pull request Apr 30, 2024
@jinge90 @kbenzie
[UR] Intercept urProgramLinkExp in ur ASAN layer (#13048)
4c7baa7 
UR part: oneapi-src/unified-runtime#1452

---------

Signed-off-by: jinge90 <ge.jin@intel.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@pbalcer pbalcer pbalcer approved these changes

@AllanZyne AllanZyne AllanZyne approved these changes

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jinge90 @codecov-commenter @pbalcer @kbenzie @AllanZyne @zhaomaosu