[PATCH] D87237: [ADT] Add ASAN Support for SmallVector

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: MaskRay.
dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:222
   static void destroy_range(T *S, T *E) {
+    const LLVM_ATTRIBUTE_UNUSED size_t Size = E - S;
     while (S != E) {
----------------
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


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:594-600
+      if (&Elt >= this->begin() && &Elt < this->end()) {
+        // If we are inserting from an element inside the SmallVector and we
+        // need to grow, the reference to the element will become invalidated on
+        // the call to grow.
+        T Copy(Elt);
+        return insert(I, std::move(Copy));
+      }
----------------
njames93 wrote:
> 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 [[ https://github.com/llvm/llvm-project/blob/2e1827271cb1c090cced7369282f9edcf9e59183/llvm/unittests/ADT/SmallVectorTest.cpp#L256 | this line ]].
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?


================
Comment at: llvm/lib/Support/Twine.cpp:54-55
   Out.pop_back();
+  // Unpoison the address of the null terminator.
+  __asan_unpoison_memory_region(Out.end(), 1);
   return StringRef(Out.data(), Out.size());
----------------
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.


================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:256-257
   // Insert at beginning
+  // TODO: Fix self referencing insertion. Right now this causes asan to fail.
   this->theVector.insert(this->theVector.begin(), this->theVector[1]);
   this->assertValuesInOrder(this->theVector, 3u, 2, 1, 2);
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87237/new/

https://reviews.llvm.org/D87237