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.

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

[ASAN] fix recursive initialization of sanitizer context #1862

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

pbalcer
Copy link
Contributor

@pbalcer pbalcer commented Jul 15, 2024

The AsanOptions class, a singleton, was being instantiated inside of the sanitizer context constructor, but, inside of its contructor, it was using the logger from inside of the context. This only worked before because the context variable was a global and the logger just so happened to be initialized prior to the AsanOptions.

Now that the layer contexts are not globals, this was causing a deadlock on trying to retrieve a context while it was being initialized. The fix is to pass the logger manually to the AsanOptions class.

A better fix might be to refactor AsanOptions to no longer be a singleton, but I was trying to minimize changes since this is blocking intel/llvm update.

The AsanOptions class, a singleton, was being instantiated inside
of the sanitizer context constructor, but, inside of its contructor,
it was using the logger from inside of the context. This only worked
before because the context variable was a global and the logger just
so happened to be initialized prior to the AsanOptions.

Now that the layer contexts are not globals, this was causing
a deadlock on trying to retrieve a context while it was being
initialized. The fix is to pass the logger manually to the
AsanOptions class.

A better fix might be to refactor AsanOptions to no longer be a static,
but I was trying to minimize changes since this is blocking intel/llvm
update.
@pbalcer pbalcer requested a review from a team as a code owner July 15, 2024 11:19
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Jul 15, 2024
@pbalcer
Copy link
Contributor Author

pbalcer commented Jul 15, 2024

2/18 Test #8: platform-adapter_native_cpu ................***Timeout 180.15 sec

This is unrelated. I've seen this failing before a few times.

@kbenzie kbenzie merged commit ddafd29 into oneapi-src:main Jul 15, 2024
52 of 54 checks passed
@pbalcer
Copy link
Contributor Author

pbalcer commented Jul 15, 2024

@AllanZyne This was failing on intel/llvm CI after #1826, and we needed to quickly fix it. So we made a decision to merge this without waiting for your review. The changes are fairly small, but feel free to make a follow-up patch if you want to refactor this code.

@yingcong-wu
Copy link
Contributor

Thank you @pbalcer , We will make a follow-up PR for this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants