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

[Michael Ilseman]

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