-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Re-enable ASAN string annotations #3164
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
moving code back and forth between msvc and stl is Fun
getting stuck on: AddressSanitizer CHECK failed: D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_poisoning.cpp:405 "((*(u8*)MemToShadow(a))) == ((0))" (0xfc, 0x0)
comment explaining what's going on at the top of _Get_asan_aligned_first_last.
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 am a bit rusty on the invariants that we provide (last years code, urghh)
_Construct_from_iter
gives me some strange feelings, as I am unsure where the repeated calls to _ASAN_STRING_REMOVE
come from
additionally, in tests, assume std::allocator is Good
tested all of them since I'm now on a powerful enough computer that that doesn't take ages
MSVC-PR-433542 has failed checks; it may be an innocuous option difference between |
Error report from MSVC-PR-433542: TL;DRThe only concerning failures (to me) are:
Full Report
|
Thanks for analyzing the MSVC-PR results! On GitHub, This repros locally and running |
@strega-nil-ms is investigating an LTCG ICE found in MSVC-internal (prod/be) testing. Moving back to WIP until that's resolved. Thanks Nicole! 🧊 🥶 🏂 |
we've had issues with them and I do not trust them.
I consider this ready for merging based on the MSVC testing! regress_amd64
OSS_x86
OSS_amd64
|
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.
LGTM after Open3d_amd64 passes :)
#define _ASAN_STRING_REMOVE(_Str) (_Str)._Remove_annotation() | ||
#define _ASAN_STRING_CREATE(_Str) (_Str)._Create_annotation() | ||
_Asan_aligned_pointers _Aligned; | ||
if constexpr (_Large_string_always_asan_aligned) { |
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.
This is essentially the same as what's in vector with the SSO stuff gone, but it's written differently (clamp is avoided in vector since everything happens under the if constexpr instead of setting _Aligned). Or am I missing something?
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.
yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?
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.
yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?
I would like the two to be as consistent as we can make them. I would also like any such changes to vector
to happen in a different PR - let's put this baby to bed.
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 the vector implementation is currently correct, I would prefer to see that changed in a followup PR, to avoid too much happening at once. (If the question were changing string to align more closely with vector, then that would be reasonable to do in this PR which is already changing string significantly.)
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.
None of these comments should block merging; we can address any of them in a followup (if at all).
#define _ASAN_STRING_REMOVE(_Str) (_Str)._Remove_annotation() | ||
#define _ASAN_STRING_CREATE(_Str) (_Str)._Create_annotation() | ||
_Asan_aligned_pointers _Aligned; | ||
if constexpr (_Large_string_always_asan_aligned) { |
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.
yeah, I could rewrite the vector stuff to more closely follow the string stuff, if you like?
I would like the two to be as consistent as we can make them. I would also like any such changes to vector
to happen in a different PR - let's put this baby to bed.
We discussed offline a bit the possibility of versioning annotations because of the SBO annotations being removed. Removing them now, and following the same ABI guarantees as the STL, might cause some headaches in the future if we try to support them. |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for overhauling this machinery and helping users find bugs in their code! 😻 🛠️ ✨ |
tested with the bugs with the previous implementation!
Fixes #3205
internal PR for testing purposes:
MSVC-PR-433542MSVC-PR-440101MSVC-PR-440279Fixes the following internal bugs: VSO-1664238, VSO-1586016, VSO-1543191