Skip to content

[UR] ASan layer fix and test update #17826

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.

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

Merged
merged 1 commit into from
Apr 7, 2025
Merged

Conversation

RossBrunton
Copy link
Contributor

Two fixes for asan:

  • The test has been updated to run on all adapters and also skip HIP
    and OpenCL (which don't support the required VirtualMem functions).
  • GetShadowMemory has been changed to CreateShadowMemory. Previously,
    shadow memory had a static lifetime, while the Context that was used
    to construct it only had a lifetime of the AsanInterceptor object.
    It has been changed and now each AsanInterceptor creates its own
    shadow memory.

Two fixes for asan:
* The test has been updated to run on all adapters and also skip HIP
  and OpenCL (which don't support the required VirtualMem functions).
* GetShadowMemory has been changed to CreateShadowMemory. Previously,
  shadow memory had a static lifetime, while the Context that was used
  to construct it only had a lifetime of the `AsanInterceptor` object.
  It has been changed and now each `AsanInterceptor` creates its own
  shadow memory.
@RossBrunton RossBrunton marked this pull request as ready for review April 3, 2025 15:07
@RossBrunton RossBrunton requested a review from a team as a code owner April 3, 2025 15:07
@RossBrunton RossBrunton requested a review from a team April 3, 2025 15:33
@RossBrunton
Copy link
Contributor Author

@intel/dpcpp-sanitizers-review Can I get this looked at when you get the chance? Just some fixes for issues I was having locally with the test.

@AllanZyne
Copy link
Contributor

Please don't change shadow memory related codes, we did this on purpose.

@pbalcer
Copy link
Contributor

pbalcer commented Apr 7, 2025

Please don't change shadow memory related codes, we did this on purpose.

But the existing code creates a global object that has a different lifetime than the loader. We should, as a general rule, avoid static lifetime on objects because it creates unnecessary complexity when it comes to cleanup and loader reinitialization. Destructor ordering is a very major problem in SYCL, and we need to be careful about not making it worse.

Can you explain why the ShadowMemory object needs to have a static lifetime?

@AllanZyne
Copy link
Contributor

Please don't change shadow memory related codes, we did this on purpose.

But the existing code creates a global object that has a different lifetime than the loader. We should, as a general rule, avoid static lifetime on objects because it creates unnecessary complexity when it comes to cleanup and loader reinitialization. Destructor ordering is a very major problem in SYCL, and we need to be careful about not making it worse.

Can you explain why the ShadowMemory object needs to have a static lifetime?

Okay, I understand what you want. @RossBrunton's modification is correct.

@RossBrunton
Copy link
Contributor Author

@intel/llvm-gatekeepers Please merge.

@martygrant martygrant merged commit acb6d1f into intel:sycl Apr 7, 2025
32 checks passed
KornevNikita pushed a commit that referenced this pull request May 27, 2025
Two fixes for asan:
* The test has been updated to run on all adapters and also skip HIP
  and OpenCL (which don't support the required VirtualMem functions).
* GetShadowMemory has been changed to CreateShadowMemory. Previously,
  shadow memory had a static lifetime, while the Context that was used
  to construct it only had a lifetime of the `AsanInterceptor` object.
  It has been changed and now each `AsanInterceptor` creates its own
  shadow memory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants