<div dir="ltr">bump with Duncan for context of recent addition of SmallVector::emplace_back which may or may not care about similar scenarios<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 11, 2014 at 1:04 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Jul 6, 2011 at 3:37 PM, Owen Anderson <<a href="mailto:resistor@mac.com">resistor@mac.com</a>> wrote:<br>
> Author: resistor<br>
> Date: Wed Jul 6 17:36:59 2011<br>
> New Revision: 134554<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=134554&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=134554&view=rev</a><br>
> Log:<br>
> Fix a subtle issue in SmallVector. The following code did not work as expected:<br>
> vec.insert(vec.begin(), vec[3]);<br>
> 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.<br>
<br>
</span>Just as a scary side-note/observation: I don't think your fix covers<br>
the case where the insertion causes a reallocation. We call grow<br>
without care at the start of the function if we need to grow - and at<br>
that point we've lost the original element that was pointed to.<br>
<br>
This turned up when I added extra checks to Copyable in r210429<br>
(reverted in r210430) which demonstrated that this operation ends up<br>
attempting to copy from an already moved-from object (which, I think<br>
in this case was leftover in the small buffer - but in other cases<br>
could be in released memory).<br>
<br>
I'm thinking a bit about how to handle this and other cases of<br>
self-insertion and trying to wrap my head around which cases we need<br>
to support, should support, etc.<br>
<br>
Figured I'd mention it in case you know you're still relying on this &<br>
might have bugs here.<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Thanks to Howard Hinnant for clarifying the correct behavior, and explaining how std::vector solves this problem.<br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/ADT/SmallVector.h<br>
> llvm/trunk/unittests/ADT/SmallVectorTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/ADT/SmallVector.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallVector.h?rev=134554&r1=134553&r2=134554&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallVector.h?rev=134554&r1=134553&r2=134554&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/ADT/SmallVector.h (original)<br>
> +++ llvm/trunk/include/llvm/ADT/SmallVector.h Wed Jul 6 17:36:59 2011<br>
> @@ -410,7 +410,14 @@<br>
> this->setEnd(this->end()+1);<br>
> // Push everything else over.<br>
> std::copy_backward(I, this->end()-1, this->end());<br>
> - *I = Elt;<br>
> +<br>
> + // If we just moved the element we're inserting, be sure to update<br>
> + // the reference.<br>
> + const T *EltPtr = &Elt;<br>
> + if (I <= EltPtr && EltPtr < this->EndX)<br>
> + ++EltPtr;<br>
> +<br>
> + *I = *EltPtr;<br>
> return I;<br>
> }<br>
> size_t EltNo = I-this->begin();<br>
><br>
> Modified: llvm/trunk/unittests/ADT/SmallVectorTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallVectorTest.cpp?rev=134554&r1=134553&r2=134554&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SmallVectorTest.cpp?rev=134554&r1=134553&r2=134554&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/ADT/SmallVectorTest.cpp (original)<br>
> +++ llvm/trunk/unittests/ADT/SmallVectorTest.cpp Wed Jul 6 17:36:59 2011<br>
> @@ -35,26 +35,26 @@<br>
> Constructable() : value(0) {<br>
> ++numConstructorCalls;<br>
> }<br>
> -<br>
> +<br>
> Constructable(int val) : value(val) {<br>
> ++numConstructorCalls;<br>
> }<br>
> -<br>
> +<br>
> Constructable(const Constructable & src) {<br>
> value = src.value;<br>
> ++numConstructorCalls;<br>
> }<br>
> -<br>
> +<br>
> ~Constructable() {<br>
> ++numDestructorCalls;<br>
> }<br>
> -<br>
> +<br>
> Constructable & operator=(const Constructable & src) {<br>
> value = src.value;<br>
> ++numAssignmentCalls;<br>
> return *this;<br>
> }<br>
> -<br>
> +<br>
> int getValue() const {<br>
> return abs(value);<br>
> }<br>
> @@ -64,7 +64,7 @@<br>
> numDestructorCalls = 0;<br>
> numAssignmentCalls = 0;<br>
> }<br>
> -<br>
> +<br>
> static int getNumConstructorCalls() {<br>
> return numConstructorCalls;<br>
> }<br>
> @@ -91,10 +91,10 @@<br>
> class SmallVectorTest : public testing::Test {<br>
> protected:<br>
> typedef SmallVector<Constructable, 4> VectorType;<br>
> -<br>
> +<br>
> VectorType theVector;<br>
> VectorType otherVector;<br>
> -<br>
> +<br>
> void SetUp() {<br>
> Constructable::reset();<br>
> }<br>
> @@ -111,7 +111,7 @@<br>
> // Assert that theVector contains the specified values, in order.<br>
> void assertValuesInOrder(VectorType & v, size_t size, ...) {<br>
> EXPECT_EQ(size, v.size());<br>
> -<br>
> +<br>
> va_list ap;<br>
> va_start(ap, size);<br>
> for (size_t i = 0; i < size; ++i) {<br>
> @@ -121,7 +121,7 @@<br>
><br>
> va_end(ap);<br>
> }<br>
> -<br>
> +<br>
> // Generate a sequence of values to initialize the vector.<br>
> void makeSequence(VectorType & v, int start, int end) {<br>
> for (int i = start; i <= end; ++i) {<br>
> @@ -155,18 +155,24 @@<br>
> theVector.push_back(Constructable(2));<br>
> assertValuesInOrder(theVector, 2u, 1, 2);<br>
><br>
> + // Insert at beginning<br>
> + theVector.insert(theVector.begin(), theVector[1]);<br>
> + assertValuesInOrder(theVector, 3u, 2, 1, 2);<br>
> +<br>
> // Pop one element<br>
> theVector.pop_back();<br>
> - assertValuesInOrder(theVector, 1u, 1);<br>
> + assertValuesInOrder(theVector, 2u, 2, 1);<br>
><br>
> - // Pop another element<br>
> + // Pop remaining elements<br>
> + theVector.pop_back();<br>
> theVector.pop_back();<br>
> assertEmpty(theVector);<br>
> -<br>
> +<br>
> // Check number of constructor calls. Should be 2 for each list element,<br>
> - // one for the argument to push_back, and one for the list element itself.<br>
> - EXPECT_EQ(4, Constructable::getNumConstructorCalls());<br>
> - EXPECT_EQ(4, Constructable::getNumDestructorCalls());<br>
> + // one for the argument to push_back, one for the argument to insert,<br>
> + // and one for the list element itself.<br>
> + EXPECT_EQ(5, Constructable::getNumConstructorCalls());<br>
> + EXPECT_EQ(5, Constructable::getNumDestructorCalls());<br>
> }<br>
><br>
> // Clear test.<br>
> @@ -198,7 +204,7 @@<br>
> SCOPED_TRACE("ResizeGrowTest");<br>
><br>
> theVector.resize(2);<br>
> -<br>
> +<br>
> // The extra constructor/destructor calls come from the temporary object used<br>
> // to initialize the contents of the resized array (via copy construction).<br>
> EXPECT_EQ(3, Constructable::getNumConstructorCalls());<br>
> @@ -226,10 +232,10 @@<br>
> for (int i = 0; i < 10; ++i) {<br>
> EXPECT_EQ(i+1, theVector[i].getValue());<br>
> }<br>
> -<br>
> +<br>
> // Now resize back to fixed size.<br>
> theVector.resize(1);<br>
> -<br>
> +<br>
> assertValuesInOrder(theVector, 1u, 1);<br>
> }<br>
><br>
> @@ -364,13 +370,13 @@<br>
><br>
> makeSequence(theVector, 1, 3);<br>
> makeSequence(otherVector, 1, 3);<br>
> -<br>
> +<br>
> EXPECT_TRUE(theVector == otherVector);<br>
> EXPECT_FALSE(theVector != otherVector);<br>
><br>
> otherVector.clear();<br>
> makeSequence(otherVector, 2, 4);<br>
> -<br>
> +<br>
> EXPECT_FALSE(theVector == otherVector);<br>
> EXPECT_TRUE(theVector != otherVector);<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>