[llvm-commits] [llvm] r134554 - in /llvm/trunk: include/llvm/ADT/SmallVector.h unittests/ADT/SmallVectorTest.cpp
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Dec 2 21:34:00 PST 2014
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());
} 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.
> 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
>
More information about the llvm-commits
mailing list