<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 2, 2014 at 9:34 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yup, it looks like `emplace_back()` has the same trouble as other growing<br>
operations, although...<br>
<span class=""><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>
</span>> to support, should support, etc. I'm not sure we should care.<br>
<br>
... I'm not sure either.<br>
<br>
It *does* look solvable, though. For `push_back()` and `emplace_back()`,<br>
instead of:<br>
<br>
if (EndX == CapacityX)<br>
grow();<br>
construct(end());<br>
setEnd(end() + 1);<br>
<br>
We need something along the lines of:<br>
<br>
if (EndX == CapacityX) {<br>
AlignedCharArray<alignof(T), sizeof(T)> Mem;<br>
construct((void *)Mem.buffer);</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
grow();<br>
uninitialized_move((T*)Mem.buffer, (T*)Mem.buffer + 1, end());<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
} else {<br>
construct(end());<br>
}<br>
setEnd(end() + 1);<br>
<br>
If it it works here, it should work for `push_back()` too.<br>
<br>
I think `insert()` could be fixed by similarly constructing the<br>
new value on the stack first, and then moving things around.<br>
<br>
The downside is that it's doing two operations (construct + move)<br>
where it could do just one (construct) in the far-more-common case<br>
of non-self-referential emplace.<br></blockquote><div><br>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.<br><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> On 2014 Dec 2, at 21:03, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> bump with Duncan for context of recent addition of SmallVector::emplace_back which may or may not care about similar scenarios<br>
><br>
> On Wed, Jun 11, 2014 at 1:04 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> 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>
> 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>
><br>
> - David<br>
><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>
><br>
<br>
</div></div></blockquote></div><br></div></div>