<div dir="ltr">On Mon, Apr 1, 2013 at 11:01 PM, Jean-Luc Duprat <span dir="ltr"><<a href="mailto:jduprat@apple.com" target="_blank" class="cremed">jduprat@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is an updated patch that fix SmallVector::insert() in all its implementations.<br></blockquote><div><br></div><div style>
You can avoid the extra branches in the non-POD case by splitting grow() into two methods (one which reallocates, and one which deallocates), and constructing the inserted elements after allocating the new buffer and before deallocating the old one. Take a look at libc++'s vector for an elegant implementation of this.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
The problem is real, it was just not very visible…<br>
<br>
Thank you for your feedback,<br>
<br>
JL<br>
<br>
<br>
<br>
<br><br>
<br>
<br>
<br>
<br>
On Apr 1, 2013, at 11:27 , Jean-Luc Duprat <<a href="mailto:jduprat@apple.com" class="cremed">jduprat@apple.com</a>> wrote:<br>
<br>
> 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.<br>
> I light of Howard's comment, SmallVector should preserve the validity of both iterators/reference in this case:<br>
> v.insert(v.begin(), v[3]);<br>
> if it is to behave similarly to std::vector.<br>
><br>
> I propose the attached fix to SmallVector.<br>
><br>
> <SmallVector.patch><br>
><br>
><br>
> JL<br>
><br>
><br>
><br>
> On Mar 27, 2013, at 13:47 , Howard Hinnant <<a href="mailto:hhinnant@apple.com" class="cremed">hhinnant@apple.com</a>> wrote:<br>
><br>
>> On Mar 27, 2013, at 4:34 PM, Dan Gohman <<a href="mailto:dan433584@gmail.com" class="cremed">dan433584@gmail.com</a>> wrote:<br>
>><br>
>>><br>
>>><br>
>>> On Wed, Mar 27, 2013 at 12:16 PM, Howard Hinnant <<a href="mailto:hhinnant@apple.com" class="cremed">hhinnant@apple.com</a>> wrote:<br>
>>> On Mar 27, 2013, at 2:44 PM, Dan Gohman <<a href="mailto:dan433584@gmail.com" class="cremed">dan433584@gmail.com</a>> wrote:<br>
>>><br>
>>>> Author: djg<br>
>>>> Date: Wed Mar 27 13:44:56 2013<br>
>>>> New Revision: 178166<br>
>>>><br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=178166&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=178166&view=rev</a><br>
>>>> Log:<br>
>>>> Avoid undefined behavior from passing a std::vector's own contents<br>
>>>> in as an argument to push_back.<br>
>>><br>
>>> The original code is not undefined behavior.<br>
>>><br>
>>> 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?<br>
>><br>
>> 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):<br>
>><br>
>>> pre: i and j are not iterators into a.<br>
>><br>
>> 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.<br>
>><br>
>> 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.<br>

>><br>
>> 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:<br>
>><br>
>>> f a function argument binds to an rvalue reference parameter, the implementation may assume that this parameter is a unique reference to this argument.<br>
>><br>
>> So, for example:<br>
>><br>
>>  v.insert(v.begin(), std::move(v[3]));<br>
>><br>
>> would be asking for trouble, whereas<br>
>><br>
>>  v.insert(v.begin(), v[3]);<br>
>><br>
>> is ok.<br>
>><br>
>> Howard<br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>