<p dir="ltr">Bumping this for me or someone to look at.</p>
<div class="gmail_quote">On Apr 3, 2013 1:42 AM, "Jean-Luc Duprat" <<a href="mailto:jduprat@apple.com">jduprat@apple.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
I am leaving the details of the relevant previous llvm-commit conversation below.<br>
<br>
iterator SmallVectorImpl::insert(iterator I, const T &Elt) and friends face two problems when the underlying storage needs to be re-allocated:<br>
1- The iterator which points into that storage needs to remain valid through the operation<br>
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.<br>
<br>
Case 1 was detected and fixed in July 2011.  I just ran into case 2, triggered by a unit test.<br>
The problem has always been there, but I was playing with non-standard memory allocations that brought this to light.<br>
We're just lucky most of the time…<br>
<br>
The conversation I was referring to below between Howard Hinnant and Dan Gohman concludes that std::vector need to specifically handle this situation.<br>
I see no reason why SmallVector would have different semantics here.<br>
<br>
The patch attached ensures that the above use case of SmallVector::insert() works as expected, without relying on luck.<br>
Please review this patch and provide feedback.<br>
<br>
<br><br>
<br>
JL<br>
<br>
<br>
<br>
On Apr 1, 2013, at 23:01 , Jean-Luc Duprat <<a href="mailto:jduprat@apple.com">jduprat@apple.com</a>> wrote:<br>
<br>
> This is an updated patch that fix SmallVector::insert() in all its implementations.<br>
> 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>
> <smv.patch><br>
><br>
><br>
><br>
><br>
> On Apr 1, 2013, at 11:27 , Jean-Luc Duprat <<a href="mailto:jduprat@apple.com">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">hhinnant@apple.com</a>> wrote:<br>
>><br>
>>> On Mar 27, 2013, at 4:34 PM, Dan Gohman <<a href="mailto:dan433584@gmail.com">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">hhinnant@apple.com</a>> wrote:<br>
>>>> On Mar 27, 2013, at 2:44 PM, Dan Gohman <<a href="mailto:dan433584@gmail.com">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">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">llvm-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div>