[llvm-commits] [llvm] r134554 - in /llvm/trunk: include/llvm/ADT/SmallVector.h unittests/ADT/SmallVectorTest.cpp
David Blaikie
dblaikie at gmail.com
Tue Dec 2 21:48:43 PST 2014
On Tue, Dec 2, 2014 at 9:34 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
> Yup, it looks like `emplace_back()` has the same trouble as other growing
> operations, although...
>
> > I'm thinking a bit about how to handle this and other cases of
> > self-insertion and trying to wrap my head around which cases we need
> > to support, should support, etc. I'm not sure we should care.
>
> ... I'm not sure either.
>
> It *does* look solvable, though. For `push_back()` and `emplace_back()`,
> instead of:
>
> if (EndX == CapacityX)
> grow();
> construct(end());
> setEnd(end() + 1);
>
> We need something along the lines of:
>
> if (EndX == CapacityX) {
> AlignedCharArray<alignof(T), sizeof(T)> Mem;
> construct((void *)Mem.buffer);
grow();
> uninitialized_move((T*)Mem.buffer, (T*)Mem.buffer + 1, end());
>
Yeah, this sounds plausible - since the type has to at least be move
constructible anyway (due to the growth), so this doesn't add extra
requirements on the operation.
> } else {
> construct(end());
> }
> setEnd(end() + 1);
>
> If it it works here, it should work for `push_back()` too.
>
> I think `insert()` could be fixed by similarly constructing the
> new value on the stack first, and then moving things around.
>
> The downside is that it's doing two operations (construct + move)
> where it could do just one (construct) in the far-more-common case
> of non-self-referential emplace.
>
Right - and it's not strictly optimal - if the grow operation was split
(allocating the new buffer first, then constructing the newly-inserted
element, then moving elements over) it could be done without extra moves.
But that's more work.
I think my head got sore right about the time I tried to figure out what,
if any, guarantees could be given for insertion of a range into itself...
when there's no reallocation happening, it's impossible (because you have
to move things out of the way first - so you might've broken the range -
indeed even a non-range insertion, if the referenced element comes after
the insertion point... I can't remember if libc++ (I looked around in there
to see how to implement these things, which things were required and which
weren't) did some sorcery to handle some subset of those cases - perhaps by
building the new element at the end, then swapping it in? I forget)
>
> > On 2014 Dec 2, at 21:03, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > bump with Duncan for context of recent addition of
> SmallVector::emplace_back which may or may not care about similar scenarios
> >
> > On Wed, Jun 11, 2014 at 1:04 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > On Wed, Jul 6, 2011 at 3:37 PM, Owen Anderson <resistor at mac.com> wrote:
> > > Author: resistor
> > > Date: Wed Jul 6 17:36:59 2011
> > > New Revision: 134554
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=134554&view=rev
> > > Log:
> > > Fix a subtle issue in SmallVector. The following code did not work as
> expected:
> > > vec.insert(vec.begin(), vec[3]);
> > > The issue was that vec[3] returns a reference into the vector, which
> is invalidated when insert() memmove's the elements down to make space.
> The method needs to specifically detect and handle this case to correctly
> match std::vector's semantics.
> >
> > Just as a scary side-note/observation: I don't think your fix covers
> > the case where the insertion causes a reallocation. We call grow
> > without care at the start of the function if we need to grow - and at
> > that point we've lost the original element that was pointed to.
> >
> > This turned up when I added extra checks to Copyable in r210429
> > (reverted in r210430) which demonstrated that this operation ends up
> > attempting to copy from an already moved-from object (which, I think
> > in this case was leftover in the small buffer - but in other cases
> > could be in released memory).
> >
> > I'm thinking a bit about how to handle this and other cases of
> > self-insertion and trying to wrap my head around which cases we need
> > to support, should support, etc.
> >
> > Figured I'd mention it in case you know you're still relying on this &
> > might have bugs here.
> >
> > - David
> >
> > >
> > > Thanks to Howard Hinnant for clarifying the correct behavior, and
> explaining how std::vector solves this problem.
> > >
> > > Modified:
> > > llvm/trunk/include/llvm/ADT/SmallVector.h
> > > llvm/trunk/unittests/ADT/SmallVectorTest.cpp
> > >
> > > Modified: llvm/trunk/include/llvm/ADT/SmallVector.h
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallVector.h?rev=134554&r1=134553&r2=134554&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/include/llvm/ADT/SmallVector.h (original)
> > > +++ llvm/trunk/include/llvm/ADT/SmallVector.h Wed Jul 6 17:36:59 2011
> > > @@ -410,7 +410,14 @@
> > > this->setEnd(this->end()+1);
> > > // Push everything else over.
> > > std::copy_backward(I, this->end()-1, this->end());
> > > - *I = Elt;
> > > +
> > > + // If we just moved the element we're inserting, be sure to
> update
> > > + // the reference.
> > > + const T *EltPtr = &Elt;
> > > + if (I <= EltPtr && EltPtr < this->EndX)
> > > + ++EltPtr;
> > > +
> > > + *I = *EltPtr;
> > > return I;
> > > }
> > > size_t EltNo = I-this->begin();
> > >
> > > Modified: llvm/trunk/unittests/ADT/SmallVectorTest.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallVectorTest.cpp?rev=134554&r1=134553&r2=134554&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/unittests/ADT/SmallVectorTest.cpp (original)
> > > +++ llvm/trunk/unittests/ADT/SmallVectorTest.cpp Wed Jul 6 17:36:59
> 2011
> > > @@ -35,26 +35,26 @@
> > > Constructable() : value(0) {
> > > ++numConstructorCalls;
> > > }
> > > -
> > > +
> > > Constructable(int val) : value(val) {
> > > ++numConstructorCalls;
> > > }
> > > -
> > > +
> > > Constructable(const Constructable & src) {
> > > value = src.value;
> > > ++numConstructorCalls;
> > > }
> > > -
> > > +
> > > ~Constructable() {
> > > ++numDestructorCalls;
> > > }
> > > -
> > > +
> > > Constructable & operator=(const Constructable & src) {
> > > value = src.value;
> > > ++numAssignmentCalls;
> > > return *this;
> > > }
> > > -
> > > +
> > > int getValue() const {
> > > return abs(value);
> > > }
> > > @@ -64,7 +64,7 @@
> > > numDestructorCalls = 0;
> > > numAssignmentCalls = 0;
> > > }
> > > -
> > > +
> > > static int getNumConstructorCalls() {
> > > return numConstructorCalls;
> > > }
> > > @@ -91,10 +91,10 @@
> > > class SmallVectorTest : public testing::Test {
> > > protected:
> > > typedef SmallVector<Constructable, 4> VectorType;
> > > -
> > > +
> > > VectorType theVector;
> > > VectorType otherVector;
> > > -
> > > +
> > > void SetUp() {
> > > Constructable::reset();
> > > }
> > > @@ -111,7 +111,7 @@
> > > // Assert that theVector contains the specified values, in order.
> > > void assertValuesInOrder(VectorType & v, size_t size, ...) {
> > > EXPECT_EQ(size, v.size());
> > > -
> > > +
> > > va_list ap;
> > > va_start(ap, size);
> > > for (size_t i = 0; i < size; ++i) {
> > > @@ -121,7 +121,7 @@
> > >
> > > va_end(ap);
> > > }
> > > -
> > > +
> > > // Generate a sequence of values to initialize the vector.
> > > void makeSequence(VectorType & v, int start, int end) {
> > > for (int i = start; i <= end; ++i) {
> > > @@ -155,18 +155,24 @@
> > > theVector.push_back(Constructable(2));
> > > assertValuesInOrder(theVector, 2u, 1, 2);
> > >
> > > + // Insert at beginning
> > > + theVector.insert(theVector.begin(), theVector[1]);
> > > + assertValuesInOrder(theVector, 3u, 2, 1, 2);
> > > +
> > > // Pop one element
> > > theVector.pop_back();
> > > - assertValuesInOrder(theVector, 1u, 1);
> > > + assertValuesInOrder(theVector, 2u, 2, 1);
> > >
> > > - // Pop another element
> > > + // Pop remaining elements
> > > + theVector.pop_back();
> > > theVector.pop_back();
> > > assertEmpty(theVector);
> > > -
> > > +
> > > // Check number of constructor calls. Should be 2 for each list
> element,
> > > - // one for the argument to push_back, and one for the list element
> itself.
> > > - EXPECT_EQ(4, Constructable::getNumConstructorCalls());
> > > - EXPECT_EQ(4, Constructable::getNumDestructorCalls());
> > > + // one for the argument to push_back, one for the argument to
> insert,
> > > + // and one for the list element itself.
> > > + EXPECT_EQ(5, Constructable::getNumConstructorCalls());
> > > + EXPECT_EQ(5, Constructable::getNumDestructorCalls());
> > > }
> > >
> > > // Clear test.
> > > @@ -198,7 +204,7 @@
> > > SCOPED_TRACE("ResizeGrowTest");
> > >
> > > theVector.resize(2);
> > > -
> > > +
> > > // The extra constructor/destructor calls come from the temporary
> object used
> > > // to initialize the contents of the resized array (via copy
> construction).
> > > EXPECT_EQ(3, Constructable::getNumConstructorCalls());
> > > @@ -226,10 +232,10 @@
> > > for (int i = 0; i < 10; ++i) {
> > > EXPECT_EQ(i+1, theVector[i].getValue());
> > > }
> > > -
> > > +
> > > // Now resize back to fixed size.
> > > theVector.resize(1);
> > > -
> > > +
> > > assertValuesInOrder(theVector, 1u, 1);
> > > }
> > >
> > > @@ -364,13 +370,13 @@
> > >
> > > makeSequence(theVector, 1, 3);
> > > makeSequence(otherVector, 1, 3);
> > > -
> > > +
> > > EXPECT_TRUE(theVector == otherVector);
> > > EXPECT_FALSE(theVector != otherVector);
> > >
> > > otherVector.clear();
> > > makeSequence(otherVector, 2, 4);
> > > -
> > > +
> > > EXPECT_FALSE(theVector == otherVector);
> > > EXPECT_TRUE(theVector != otherVector);
> > > }
> > >
> > >
> > > _______________________________________________
> > > 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/20141202/39977c24/attachment.html>
More information about the llvm-commits
mailing list