SmallVector::insert() fix

[David Blaikie]

Bumping this for me or someone to look at.
On Apr 3, 2013 1:42 AM, "Jean-Luc Duprat" <jduprat at apple.com> wrote:

> 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.
> I am leaving the details of the relevant previous llvm-commit conversation
> below.
>
> iterator SmallVectorImpl::insert(iterator I, const T &Elt) and friends
> face two problems when the underlying storage needs to be re-allocated:
> 1- The iterator which points into that storage needs to remain valid
> through the operation
> 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.
>
> Case 1 was detected and fixed in July 2011.  I just ran into case 2,
> triggered by a unit test.
> The problem has always been there, but I was playing with non-standard
> memory allocations that brought this to light.
> We're just lucky most of the time…
>
> The conversation I was referring to below between Howard Hinnant and Dan
> Gohman concludes that std::vector need to specifically handle this
> situation.
> I see no reason why SmallVector would have different semantics here.
>
> The patch attached ensures that the above use case of
> SmallVector::insert() works as expected, without relying on luck.
> Please review this patch and provide feedback.
>
>
>
>
> JL
>
>
>
> On Apr 1, 2013, at 23:01 , 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
>
>
> _______________________________________________
> 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/20130428/cb0a9451/attachment.html>