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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

I believe I filed the second bug ( https://bugs.llvm.org/show_bug.cgi?id=16253 ) a while back because I tried to do it myself (admittedly, it was myself 7 years ago, so maybe my perspective has changed over the years) and found it too difficult. That there were corner cases or something that made me unsure how to move forward - so I filed the bug and forgot about it.

Wish I'd written down more of what those corner cases were... let's see what I can construct from what I did say in the bug.

r183465 <https://github.com/llvm/llvm-project/commit/f84a03a589acadcbf07d1c4a92f132268781e674>: Clearly a workaround for this bug (or a fix for an incorrect use of SmallVector, if we deem this use to be out of contract for SmallVector), though a somewhat confusing/unclear commit message to justify it.
r183459 <https://github.com/llvm/llvm-project/commit/3bd2524b601c6c5de28d931705bf66ce2dd0fc4a>: Another of the same

Hmm - I think maybe what I had trouble with was understanding how much this shuold generalize. Should this handle subranges? (if you try to append half of a vector onto itself) and if so, how? Anyone happen to know what's guaranteed by the standard (perhaps only push_back of single elements - that's easier to handle)

In D87326#2263471 <https://reviews.llvm.org/D87326#2263471>, @njames93 wrote:

> For the clang tidy naming warnings, is it better to follow the warnings, or the current convention of the file?

Current convention of the file, thanks!

> I haven't added specific test cases for this, but all ADT Tests did run without a hitch under asan

Did they run without a hitch under asan before the change too? I imagine so.

So probably worth adding specific tests for overlapping - ones that would've failed under asan before this change, but pass with it.

The test updates you have done in the patch so far - they're intended to make the test coverage a bit more robust to have more confidence over the refactoring? Could those test changes be committed first/separately - I assume they're intended to pass before and after the patch? So might be good as a preliminary/separate patch before this one.



================
Comment at: llvm/include/llvm/ADT/SmallVector.h:323
+    // Destory the items in the old vector.
+    std::for_each(Vec.begin(), Vec.end(), [](T &I) { I.~T(); });
+    this->take_buffers(Vec);
----------------
Prefer range-based-for rather than std::for_each, probably (here and below)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326