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

[DeviceAsan] Symbolizer may destruct before SanitizerInterceptor #2237

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

yingcong-wu
Copy link
Contributor

We may try to use symbolizer in the destructor of SanitizerInterceptor, but since we are using a static Symbolizer, we cannot control it's destruction order and it can be destructed before SanitizerInterceptor layer. This patch tries to manually let the Symboilzer destruct at the very end of the exit process.

@yingcong-wu yingcong-wu requested a review from a team as a code owner October 24, 2024 07:11
@github-actions github-actions bot added the loader Loader related feature/bug label Oct 24, 2024
@yingcong-wu yingcong-wu changed the title [DeviceAsan] Symbolizer may destruct before SanitierInterceptor [DeviceAsan] Symbolizer may destruct before SanitizerInterceptor Oct 24, 2024
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

@yingcong-wu yingcong-wu force-pushed the yc/1024-devasan-bug-symbolizer-destruct-too-early branch from e42beea to 621cb2b Compare October 25, 2024 02:05
@github-actions github-actions bot added the sanitizer Sanitizer layer issues/changes/specification label Oct 25, 2024
@yingcong-wu
Copy link
Contributor Author

Hi @pbalcer , we have another internal discussion and we still think having a symbolizer within the context is strange and we don't want that. Instead, we will construct a symbolizer each time we try to use it. Symbolization happened in the error reporting stage and we don't think performance is a problem at that stage.

By doing so we won't have the symbolizer in the context and won't have the destruction order problem too.

@pbalcer
Copy link
Contributor

pbalcer commented Oct 25, 2024

Hi @pbalcer , we have another internal discussion and we still think having a symbolizer within the context is strange and we don't want that. Instead, we will construct a symbolizer each time we try to use it. Symbolization happened in the error reporting stage and we don't think performance is a problem at that stage.

By doing so we won't have the symbolizer in the context and won't have the destruction order problem too.

Sure, makes sense. Another option could be using the AtomicSingleton type - https://github.com/oneapi-src/unified-runtime/blob/main/source/common/ur_util.hpp#L427, but that might be an overkill.

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.

4 participants