[ADT] Add ASAN Support for SmallVector

[njames93]

Summary

Mark addresses that are currently unused in a SmallVector as poisoned. This can help catch some iterator invalidation bugs when the SmallVector grows.

Running the ADTTest suite found an iterator invalidation bug in SmallVectorImpl<T>::insert(iterator, const T&) when the item to insert is contained inside the SmallVector.

[njames93]

llvm/include/llvm/ADT/SmallVector.h

594–595

Is this check the right behaviour, or is inserting an item from a SmallVector into itself considered UB?
If that would be the right behaviour then there is case to also add this check to append, push_back and resize.
If its classed as UB then the test case SmallVectorTest.PushPopTest needs addressing, specifically this line.

[njames93]

Undo fix for self referencing insertion

[njames93]

Undo SmallVectorTest fix

[RKSimon]

Adding some potential reviewers

[dblaikie]

llvm/include/llvm/ADT/SmallVector.h

222

is attribute unused the right one to use here?

(also LLVM doesn't usually use top-level const on local variables)

I'd probably add a (void)Size; down next to the poison call, like we'd use with variables only used in assertions

594–595

Not sure I understand the question - Was this comment related to a different line of code?

Ah, maybe the 608 "if (I <= EltPtr", etc bit?

I think that's the one @MaskRay pointed out on the self-insert vector review, the commit message for the commit that added this support seems to indicate that the at-the-time libc++ maintainer suggested this was valid for std::vector, and was implemented in a similar way there - but it would be good to check on the implementation strategy and be able to quote some words from the standard to back this up. Could you check those things?

llvm/lib/Support/Twine.cpp

54–55

Do existing LLVM unit tests fail under ASan due to this unpoison being missing? Or is this untested? If it's untested, it'd be good to add some test coverage for it (even if those tests only fail under asan and so don't fail for most/other uses)

Similarly for the SmallString change earlier.

llvm/unittests/ADT/SmallVectorTest.cpp

256–257

Is the intent to fix this in a later commit? Would that then make this change cause asan buildbots/builds to fail when they aren't currently? (that wouldn't be good/acceptable - so if that's the case, we'll need to decide on a fix before this is committed)

[njames93]

In D87237#2270189, @RKSimon wrote:

Adding some potential reviewers

Should point out this is current state is not intended for review, it was for context in a mailing list. Once some other issues are resolved I'll do this using the asan sequential container operations.