[PATCH] D87326: [ADT] Fix reference invalidation when self referencing a SmallVector

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

>>> then when the call to insert etc is made the container grows a second time. In that case the element to insert would now be in freed memory which would cause asan to fail. However right now that test case doesn't seem to appear, it does happen where the container grows from small size to insert. but as the memory is stack based there, its fine, from asans POV, to reference it.
>>> In D87237 <https://reviews.llvm.org/D87237>, I experimented by putting asan instrumentation inside SmallVector, which caused a test case to fail when ran under asan.
>>
>> OK, so I think you're saying - the current in-tree tests, and the tests you're adding, currently don't fail even though they do test self-insertion only because they're testing in small -> big (not big -> big) mode? But with ASan instrumented SmallVector, even a small->big would cause test failures of the existing tests, without this patch to fix self-insertion?
>
> The tests inside `Constructable` will cause the current in-tree self-insertion test to fail as that will detect copying from a destructed value in the call to insert. The asan instrumented SmallVector would also fail because it will detect accessing the poisoned inline buffer in the small->big growth. However with this patch both of these causes of failing tests will be fixed.

Ah, makes sense - sweet!

> Use global namespace new inside GrowBuffer.

Why this change? Would've thought op new overloads might be desirable, but I certainly don't know the specifics off-hand right now.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:305-306
+  template <typename... ArgTypes> void emplace(ArgTypes &&...Args) {
+    assert(this->size() < this->capacity() && "Overflowed GrowBuffer storage");
+    ::new ((void *)this->end()) T(std::forward<ArgTypes>(Args)...);
+    ++this->Size;
----------------
Do these (and other) member function calls need to be qualified with "this->"? If not, please remove that. (usually only see that needed when it's a call into a dependent base class, right?)


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:334
+    // Destory the items in the old vector.
+    llvm::for_each(Vec, [](T &I) { I.~T(); });
+    this->take_buffers(Vec);
----------------
Generally prefer range-based-for loops rather than std/llvm::for_each (about the only time I might suggest for_each would be if you had an existing lambda to pass)


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:706
+      std::fill(this->begin(), this->end(), Elt);
+    } else if (NumElts < this->size()) {
+      this->destroy_range(this->begin() + NumElts, this->end());
----------------
what was wrong/happening in the absence of these "else" clauses? were tests failing? I guess the fill was happening twice, maybe? That should've failed the tests with the tighter constraints you added, right? Did it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326