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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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! :)

> In D87326#2269802 <https://reviews.llvm.org/D87326#2269802>, @dblaikie wrote:
>
>>> They did but that was only because the test cases don't grow multiple times from what I can see
>>
>> Hmm - I thought this was about self-referential inserts/push_back, etc? Was/is there also bugs related to multiple growth? Could you show a small test case that'd fail asan with the current in-tree code related to multiple growth?
>
> When I say grow multiple times, I mean the container has already grown once,

You mean the container is in non-small mode? (it might not've grown to get there - it might've been initialized with too much data to fit in the small buffer - though I guess that still might look like "growth" so we're probably on the same page 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87326