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

[Richard Smith]

On Mon, 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.
>

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.


> 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
>
>
>
>
>
>
>
>
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130402/ef92fe34/attachment.html>