[ADT] Adding instrumentation for ASAN to SmallVector

Dear list,

I recently tried to add instrumentation to SmallVector for using
Address sanitizer to detect cases where references used after they are
invalidated. This basic implementation for this is here -
https://reviews.llvm.org/D87237

However, in adding/testing this, I did uncover some questionable code.
Firstly `SmallString<unsigned>::c_str()` and
`Twine::toNullTerminatedStringRef(SmallVectorImpl<char>&)` both use
bytes outside the range of the SmallVectors storage. This isn't
inherently bad.
Secondly calling `SmallVectorImpl<T>::insert(iterator, const T&)`
results in a reference invalidation when the element to insert is
contained inside the SmallVector and the SmallVector needs to grow for
the insert. This has been fixed inside the aforementioned PR.

My main point here is how does everyone feel about using ASAN to catch
bugs like this not only inside SmallVector but also adding the
instrumentation to some other containers used by llvm. If people are
happy with this implementation for SmallVector I'd be happy for
feedback on the PR. It would likely need some specific asan test cases
however I'm not entirely sure how to go about adding those.

Thanks for reading,

~Nathan

It sounds like a very reasonable strategy!

Have you seen __sanitizer_annotate_contiguous_container (compiler-rt/include/sanitizer/common_interface_defs.h)? In case you missed it - we have container-overflow built into ASan and implemented in std::vector in libcxx, and this is the canonical way of annotating containers like this.

Have you tried building LLVM/Clang with an ASan-ified LLVM/Clang? Curious to see whether there’s any latent bugs around.

My main concern with container poisoning is that it is all-or-nothing - you need to build all translation units that include the container definition with ASan, or risk false positive due to a missing shadow update. This is not a property of ASan in general, and can result in confusing error reports. I’m fine with it as long as it can be easily disabled, see below.

It sounds like a very reasonable strategy!

Have you seen __sanitizer_annotate_contiguous_container (compiler-rt/include/sanitizer/common_interface_defs.h)? In case you missed it - we have container-overflow built into ASan and implemented in std::vector in libcxx, and this is the canonical way of annotating containers like this.

Yes, this will produce better error reports, and it can be disabled with a runtime flag.