-
Notifications
You must be signed in to change notification settings - Fork 22
Add ASAN support #158
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
Add ASAN support #158
Conversation
…sanitizer configuration
…ain_import_asan_runtime_closure_feature
…ed by asan_runtime_closure_feature)
| ], | ||
| ) | ||
|
|
||
| FEATURES_ESSENTIAL = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it's declared twice for CUDA and non-CUDA implementation. The majority of the entries is intersecting with each other. Do you think it's worth moving it in some separate list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our toolchains are still under active development, I recommend deferring this optimization until the feature set stabilizes. Once we have implemented the core functionality and shifted our focus to long-term support, we can address optimizations without the risk of shifting requirements between releases.
| ], | ||
| )) | ||
|
|
||
| linker_dir_flags = depset([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was it done this way for the readability? or can it be changed like this?
toolchain_libraries = toolchain_import_info.linking_context.static_libraries.to_list() +
toolchain_import_info.linking_context.dynamic_libraries.to_list() +
toolchain_import_info.linking_context.additional.to_list()
linker_dir_flags = depset([
"-L" + file.dirname
for file in toolchain_libraries
]).to_list()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but the current version looks a little bit more readable to me because it doesn't have long lines of code and I can easily read what types of libraries were handled.
|
Hi Yulia, |
This Pull Request introduces AddressSanitizer (ASAN) support for the Linux x86_64 and Linux x86_64 CUDA toolchains. These changes have been verified through integration testing with the XLA and JAX projects.
Current Limitations & Maintenance
clang-with-sanitizer: This feature will remain available for the time being to provide additional diagnostic capabilities during the transition.
Planned Improvements
Provide a comprehensive technical description for the clang-with-sanitizer feature.
Introduce a wrapper function to simplify the import of sanitized toolchains and provide detailed configuration guidelines.