[llvm-commits] [llvm] r134554 - in /llvm/trunk: include/llvm/ADT/SmallVector.h unittests/ADT/SmallVectorTest.cpp

[David Blaikie]

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