-
Notifications
You must be signed in to change notification settings - Fork 75
[SWDEV-563823][Compiler-rt][ASan] Simplify API Logic 'asan_hsa_amd_ipc_memory_create'. #475
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
base: amd-staging
Are you sure you want to change the base?
[SWDEV-563823][Compiler-rt][ASan] Simplify API Logic 'asan_hsa_amd_ipc_memory_create'. #475
Conversation
971d852 to
0c173eb
Compare
0c173eb to
c1765c0
Compare
|
Does the test application in the ticket print the same "base" "len" and "end" as the uninstrumented version (taking into account address randomization)? |
Yes. If we include the current fix with/without intercepting hsa_amd_pointer_info. Non-Instrumented O/PInstrumented O/P |
|
But the output in the ticket, when -fsanitize=address is not used is: hsa_amd_pointer_info: test_buff 0x7f3df2400000 base 0x7f3df2400000 len 1024 end 0x7f3df2400400 Here the buffer and the base are the same, and len is 1024. Shouldn't our intercepted version of hsa_amd_pointer_info return the same buffer and base, and a len of 1024 instead of 16384? |
Hi @b-sumner , Ok in my previous o/p, the problem was because of LD_LIBRARY_PATH set to asan instrumented libraries and I compiled non-instrumented o/p without flags If I avoid adding the I think instrumentation here is not kind of problem, we enable the asan interception of those hsa_amd* api calls in the asanified libraries only as our code is bounded by preprocessor macro |
|
Hi @b-sumner, Here are the results without Instrumented (ASan enabled via -fsanitize=address -shared-libsan with LD_LIBRARY_PATH set to path/to/lib/asan)Non-Instrumented (ASan disabled with LD_LIBRARY_PATH not set to /path/to/lib/asan) |
|
Thanks! My thinking is that in order to provide acceptable information to our interception of hsa_amd_ipc_memory_create, our interception of hsa_amd_pointer_info should return the same base, len, and end as observed when the application is not built with instrumentation (or running instrumented libraries). But I would like to hear @bing-ma's take on this. |
c1765c0 to
0bb451f
Compare
0bb451f to
9bc5e55
Compare
9bc5e55 to
339e178
Compare
'asan_hsa_amd_ipc_memory_create'.
- Use reinterpret_cast<uptr> for pointer arithmetic.
- Add sanitizer interception logic for api 'hsa_amd_pointer_info'.
- Allow only valid values of ptr and len in non-ASan mode.
- ptr == Actual agentBaseAddress && len ==
original_len_used_in_alloc
- Allow only valid values of ptr and len in ASan mode. pinfo resembles
to pointer info extracted in GetBlockBegin function.
- ptr == pinfo.agentBaseAddress && len == pinfo.sizeInBytes
- ptr == original_ptr_returned_by_ASAN && len ==
original_len_used_in_alloc
Improve logic of asan_hsa_amd_ipc_memory_create based on observations.
69a471e to
ee235e3
Compare
suggestions. This commit again adds the interception of 'hsa_amd_pointer_info' call.
ee235e3 to
8681926
Compare
afae2e3 to
619e575
Compare
| uint32_t* num_agents_accessible, | ||
| hsa_agent_t** accessible) { | ||
| void* ptr_ = get_allocator().GetBlockBegin(ptr); | ||
| AsanChunk* m = instance.GetAsanChunkByAddr(reinterpret_cast<uptr>(ptr_)); |
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.
Do we not need to guard against null ptr_ here like we do in ipc create?
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.
If ptr_ == nullptr then if condition at line number 1559 will fail.
Then if condition at line number 1562 for first condition status == HSA_STATUS_SUCCESS would fail ambiguously as status is not getting set up with default value. I think we need to initialize this with status=HSA_STATUS_ERROR_NOT_INITIALIZED. Is that ok @b-sumner && @bing-ma?
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 default initialized status with value HSA_STATUS_ERROR_NOT_INITIALIZED in 00898ca.
- Changes to 'asan_hsa_amd_pointer_info'
- Initialize status with default value
'HSA_STATUS_ERROR_NOT_INITIALIZE'.
original_len_used_in_alloc
to pointer info extracted in GetBlockBegin function.
original_len_used_in_alloc