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

[Nathan James via Phabricator via llvm-commits]

njames93 added a comment.

In D87326#2270332 <https://reviews.llvm.org/D87326#2270332>, @dblaikie wrote:

> In D87326#2270142 <https://reviews.llvm.org/D87326#2270142>, @njames93 wrote:
>
>> In D87326#2269796 <https://reviews.llvm.org/D87326#2269796>, @MaskRay wrote:
>>
>>> Yeah, seems that SmallVector::push_back should be fixed to allow `a.push_back(a[0])`. SmallVector::insert has been fixed in rL134554 <https://reviews.llvm.org/rL134554>
>>
>> It hasn't, that handles the case where the element to insert is moved when shifting the elements down to make room, but not when the element to insert is moved because of container growth.
>
> Yeah, agreed. rL134554 <https://reviews.llvm.org/rL134554> was only half the fix (fixing the non-growth case). Your/this patch now fixes the other half (the growth case) and fixes the push_back case (which only has a problem on growth, doesn't have a problem on non-growth, unlike insert). Great! :)

Yeah thats whats happening here.

>> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326