SmallVector::insert() fix

Tue Apr 2 07:40:31 PDT 2013

I realize I may not have been very clear in proposing this patches for review by tacking on to a previous conversation, so let me start again.
I am leaving the details of the relevant previous llvm-commit conversation below.

iterator SmallVectorImpl::insert(iterator I, const T &Elt) and friends face two problems when the underlying storage needs to be re-allocated:
1- The iterator which points into that storage needs to remain valid through the operation
2 - If the element to be inserted in the vector is already in the vector, say v.insert(v.begin(), v[3]), then the reference needs to remain valid across the grow() operation.

Case 1 was detected and fixed in July 2011.  I just ran into case 2, triggered by a unit test.  
The problem has always been there, but I was playing with non-standard memory allocations that brought this to light.
We're just lucky most of the time…

The conversation I was referring to below between Howard Hinnant and Dan Gohman concludes that std::vector need to specifically handle this situation.
I see no reason why SmallVector would have different semantics here.

The patch attached ensures that the above use case of SmallVector::insert() works as expected, without relying on luck.
Please review this patch and provide feedback.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smv.patch
Type: application/octet-stream
Size: 6257 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130402/26e38473/attachment.obj>
-------------- next part --------------

JL

On Apr 1, 2013, at 23:01 , Jean-Luc Duprat <jduprat at apple.com> wrote:

> This is an updated patch that fix SmallVector::insert() in all its implementations.
> This bug was being triggered for some allocation sizes on Linux by the test suite, although not for the default allocs in the LLVM tree.
> The problem is real, it was just not very visible…
> 
> Thank you for your feedback,
> 
> JL
> 
> 
> 
> <smv.patch>
> 
> 
> 
> 
> On Apr 1, 2013, at 11:27 , Jean-Luc Duprat <jduprat at apple.com> wrote:
> 
>> I just ran into a similar issue with LLVM's SmallVector in case that was very similar to the one discussed in the standard library.
>> I light of Howard's comment, SmallVector should preserve the validity of both iterators/reference in this case:
>> v.insert(v.begin(), v[3]);
>> if it is to behave similarly to std::vector.
>> 
>> I propose the attached fix to SmallVector.
>> 
>> <SmallVector.patch>
>> 
>> 
>> JL
>> 
>> 
>> 
>> On Mar 27, 2013, at 13:47 , Howard Hinnant <hhinnant at apple.com> wrote:
>> 
>>> On Mar 27, 2013, at 4:34 PM, Dan Gohman <dan433584 at gmail.com> wrote:
>>> 
>>>> 
>>>> 
>>>> On Wed, Mar 27, 2013 at 12:16 PM, Howard Hinnant <hhinnant at apple.com> wrote:
>>>> On Mar 27, 2013, at 2:44 PM, Dan Gohman <dan433584 at gmail.com> wrote:
>>>> 
>>>>> Author: djg
>>>>> Date: Wed Mar 27 13:44:56 2013
>>>>> New Revision: 178166
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=178166&view=rev
>>>>> Log:
>>>>> Avoid undefined behavior from passing a std::vector's own contents
>>>>> in as an argument to push_back.
>>>> 
>>>> The original code is not undefined behavior.
>>>> 
>>>> My interpretation was that the push_back could cause reallocation which could invalidate the reference to the old element before it is read. Is there a guarantee that this won't happen?
>>> 
>>> There is not an allowance for the vendor to let it happen.  Such an allowance would look like the one for insert(p, i, j):
>>> 
>>>> pre: i and j are not iterators into a.
>>> 
>>> In practice, the new buffer has to be allocated and populated before the old buffer is deallocated, so there is no hardship on the vendor anyway.
>>> 
>>> The vendor does have to be careful with vector::insert(iterator, const T&), as self-referencing can be visible there.  But again in that case the standard puts the onus on the vendor, not on the client to just make things work.
>>> 
>>> Note that if you had moved from the source that would be another matter as 17.6.4.9 [res.on.arguments]/p1/b3 says:
>>> 
>>>> f a function argument binds to an rvalue reference parameter, the implementation may assume that this parameter is a unique reference to this argument.
>>> 
>>> So, for example:
>>> 
>>> v.insert(v.begin(), std::move(v[3]));
>>> 
>>> would be asking for trouble, whereas 
>>> 
>>> v.insert(v.begin(), v[3]);
>>> 
>>> is ok.
>>> 
>>> Howard
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits