[PATCH] D91837: ADT: Fix reference invalidation in SmallVector APIs that pass in a value

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D91837#2469329 <https://reviews.llvm.org/D91837#2469329>, @dexonsmith wrote:

> Thanks for the review!
>
> A few responses inline; I'll try to update the patch tomorrow.
>
> Regarding number of patches, this is already split into multiple patches in my local tree:
>
> 1. Fix invalidation for `push_back` and `insert` (one), the latter of which sometimes calls `push_back`.
> 2. Fix invalidation for `append` and `insert` (N), the latter of which sometimes calls `append`.
> 3. Fix invalidation for `resize`.
>
> (The various helpers get added as needed.)
>
> Happy to split it now that the performance has been validated. I'll probably find time to rebase and post tomorrow -- let me know if there's something else you'd like split out.

Makes sense/sounds good.

> In D91837#2468608 <https://reviews.llvm.org/D91837#2468608>, @dblaikie wrote:
>
>> Oh, one other question: How's all this implementation strategy compared to std::vector? Be nice to be relying on prior art where possible.
>
> Libc++'s `std::vector` uses a more general, split buffer approach that fixes all the invalidation, quite similar to https://reviews.llvm.org/D87326. Unfortunately that was reverted due to compile-time and code size regressions. My thinking is that we can try to land https://reviews.llvm.org/D87326 again after this patch (series) to fix the remaining invalidation (and that the regressions may be less bad / acceptable since the common cases in this patch are handled in a more lightweight manner).

*thumbs up*



================
Comment at: llvm/unittests/ADT/SmallVectorTest.cpp:1083-1085
+  // Append enough more elements that V will grow again. This time the old
+  // storage will have been freed at time of access and sanitizers can catch
+  // the use-after-free.
----------------
dexonsmith wrote:
> dblaikie wrote:
> > By this you mean "sanitizers would catch use-after-free if this support were missing/broken"? Otherwise this reads as though it might be a sanitizer-only death test, specifically checking that misuse fails/is diagnosed, which it isn't.
> Correct; not a death test, but I expect the sanitizers to catch the problem if the implementation is wrong.
Would probably be good to rephrase this comment (& elsewhere the same text appears) to clarify. I'd find this confusing if/when I come across it in the future.


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

https://reviews.llvm.org/D91837