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

[Jean-Luc Duprat]

Michael your first two points are noted and will be addressed.

As to your last point, the following template is generally safe:
 template<typename ItTy>
  iterator insert(iterator I, ItTy From, ItTy To) 

Where ItTy is a different type than iterator, it shouldn't point within the Small vector.  However when the template 
is instantiated with IyTy = iterator, we get the following:

iterator insert(iterator I, iterator From, iterator To)

and in this case we need to handle the possibility that From and To are pointing within the SmallVector.
Hence there is an instantiation of the method that covers that case.


JL



On Apr 2, 2013, at 9:50 , Michael Ilseman <milseman at apple.com> wrote:

> Do you have a way to expose the bug on OSX as well, or is it only reproducible on Linux? Review below:
> 
> +    size_t EPos = size_t(-1);
> 
> I think the canonical LLVM way to do this is ~0U
> 
> +    if (I <= EltPtr && EltPtr < this->EndX)
> 
> Why is this when EltPtr is between I and end() rather than between begin() and end()?
> 
> Also, looking at the diff it seems like you're adding a new iterator-based insert method, rather than revising an existing one, is this intended?
> 
> On Apr 1, 2013, at 11:01 PM, 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
>