[llvm] r178166 - Avoid undefined behavior from passing a std::vector's own contents

Jean-Luc Duprat jduprat at apple.com
Mon Apr 1 23:01:24 PDT 2013


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



-------------- 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/20130401/11a88fe3/attachment.obj>
-------------- next part --------------





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



More information about the llvm-commits mailing list