diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h index b9c30abcb579e3..78d0848b1fcc22 100644 --- a/llvm/include/llvm/ADT/SmallVector.h +++ b/llvm/include/llvm/ADT/SmallVector.h @@ -220,24 +220,6 @@ class SmallVectorTemplateCommon } void assertSafeToEmplace() {} - /// Reserve enough space to add one element, and return the updated element - /// pointer in case it was a reference to the storage. - template - static const T *reserveForAndGetAddressImpl(U *This, const T &Elt, size_t N) { - size_t NewSize = This->size() + N; - if (LLVM_LIKELY(NewSize <= This->capacity())) - return &Elt; - - bool ReferencesStorage = false; - int64_t Index = -1; - if (LLVM_UNLIKELY(This->isReferenceToStorage(&Elt))) { - ReferencesStorage = true; - Index = &Elt - This->begin(); - } - This->grow(NewSize); - return ReferencesStorage ? This->begin() + Index : &Elt; - } - public: using size_type = size_t; using difference_type = ptrdiff_t; @@ -321,12 +303,7 @@ template ::value) && (is_trivially_move_constructible::value) && std::is_trivially_destructible::value> class SmallVectorTemplateBase : public SmallVectorTemplateCommon { - friend class SmallVectorTemplateCommon; - protected: - static constexpr bool TakesParamByValue = false; - using ValueParamT = const T &; - SmallVectorTemplateBase(size_t Size) : SmallVectorTemplateCommon(Size) {} static void destroy_range(T *S, T *E) { @@ -356,31 +333,20 @@ class SmallVectorTemplateBase : public SmallVectorTemplateCommon { /// element, or MinSize more elements if specified. void grow(size_t MinSize = 0); - /// Reserve enough space to add one element, and return the updated element - /// pointer in case it was a reference to the storage. - const T *reserveForAndGetAddress(const T &Elt, size_t N = 1) { - return this->reserveForAndGetAddressImpl(this, Elt, N); - } - - /// Reserve enough space to add one element, and return the updated element - /// pointer in case it was a reference to the storage. - T *reserveForAndGetAddress(T &Elt, size_t N = 1) { - return const_cast(this->reserveForAndGetAddressImpl(this, Elt, N)); - } - - static T &&forward_value_param(T &&V) { return std::move(V); } - static const T &forward_value_param(const T &V) { return V; } - public: void push_back(const T &Elt) { - const T *EltPtr = reserveForAndGetAddress(Elt); - ::new ((void *)this->end()) T(*EltPtr); + this->assertSafeToAdd(&Elt); + if (LLVM_UNLIKELY(this->size() >= this->capacity())) + this->grow(); + ::new ((void*) this->end()) T(Elt); this->set_size(this->size() + 1); } void push_back(T &&Elt) { - T *EltPtr = reserveForAndGetAddress(Elt); - ::new ((void *)this->end()) T(::std::move(*EltPtr)); + this->assertSafeToAdd(&Elt); + if (LLVM_UNLIKELY(this->size() >= this->capacity())) + this->grow(); + ::new ((void*) this->end()) T(::std::move(Elt)); this->set_size(this->size() + 1); } @@ -430,18 +396,7 @@ void SmallVectorTemplateBase::grow(size_t MinSize) { /// skipping destruction. template class SmallVectorTemplateBase : public SmallVectorTemplateCommon { - friend class SmallVectorTemplateCommon; - protected: - /// True if it's cheap enough to take parameters by value. Doing so avoids - /// overhead related to mitigations for reference invalidation. - static constexpr bool TakesParamByValue = sizeof(T) <= 2 * sizeof(void *); - - /// Either const T& or T, depending on whether it's cheap enough to take - /// parameters by value. - using ValueParamT = - typename std::conditional::type; - SmallVectorTemplateBase(size_t Size) : SmallVectorTemplateCommon(Size) {} // No need to do a destroy loop for POD's. @@ -482,25 +437,12 @@ class SmallVectorTemplateBase : public SmallVectorTemplateCommon { /// least one more element or MinSize if specified. void grow(size_t MinSize = 0) { this->grow_pod(MinSize, sizeof(T)); } - /// Reserve enough space to add one element, and return the updated element - /// pointer in case it was a reference to the storage. - const T *reserveForAndGetAddress(const T &Elt, size_t N = 1) { - return this->reserveForAndGetAddressImpl(this, Elt, N); - } - - /// Reserve enough space to add one element, and return the updated element - /// pointer in case it was a reference to the storage. - T *reserveForAndGetAddress(T &Elt, size_t N = 1) { - return const_cast(this->reserveForAndGetAddressImpl(this, Elt, N)); - } - - /// Copy \p V or return a reference, depending on \a ValueParamT. - static ValueParamT forward_value_param(ValueParamT V) { return V; } - public: - void push_back(ValueParamT Elt) { - const T *EltPtr = reserveForAndGetAddress(Elt); - memcpy(reinterpret_cast(this->end()), EltPtr, sizeof(T)); + void push_back(const T &Elt) { + this->assertSafeToAdd(&Elt); + if (LLVM_UNLIKELY(this->size() >= this->capacity())) + this->grow(); + memcpy(reinterpret_cast(this->end()), &Elt, sizeof(T)); this->set_size(this->size() + 1); } @@ -520,9 +462,6 @@ class SmallVectorImpl : public SmallVectorTemplateBase { using size_type = typename SuperClass::size_type; protected: - using SmallVectorTemplateBase::TakesParamByValue; - using ValueParamT = typename SuperClass::ValueParamT; - // Default ctor - Initialize to empty. explicit SmallVectorImpl(unsigned N) : SmallVectorTemplateBase(N) {} @@ -563,7 +502,7 @@ class SmallVectorImpl : public SmallVectorTemplateBase { /// Like resize, but \ref T is POD, the new values won't be initialized. void resize_for_overwrite(size_type N) { resizeImpl(N); } - void resize(size_type N, ValueParamT NV) { + void resize(size_type N, const T &NV) { if (N == this->size()) return; @@ -572,8 +511,11 @@ class SmallVectorImpl : public SmallVectorTemplateBase { return; } - // N > this->size(). Defer to append. - this->append(N - this->size(), NV); + this->assertSafeToReferenceAfterResize(&NV, N); + if (this->capacity() < N) + this->grow(N); + std::uninitialized_fill(this->end(), this->begin() + N, NV); + this->set_size(N); } void reserve(size_type N) { @@ -609,9 +551,12 @@ class SmallVectorImpl : public SmallVectorTemplateBase { } /// Append \p NumInputs copies of \p Elt to the end. - void append(size_type NumInputs, ValueParamT Elt) { - const T *EltPtr = this->reserveForAndGetAddress(Elt, NumInputs); - std::uninitialized_fill_n(this->end(), NumInputs, *EltPtr); + void append(size_type NumInputs, const T &Elt) { + this->assertSafeToAdd(&Elt, NumInputs); + if (NumInputs > this->capacity() - this->size()) + this->grow(this->size()+NumInputs); + + std::uninitialized_fill_n(this->end(), NumInputs, Elt); this->set_size(this->size() + NumInputs); } @@ -677,12 +622,6 @@ class SmallVectorImpl : public SmallVectorTemplateBase { private: template iterator insert_one_impl(iterator I, ArgType &&Elt) { - // Callers ensure that ArgType is derived from T. - static_assert( - std::is_same>, - T>::value, - "ArgType must be derived from T!"); - if (I == this->end()) { // Important special case for empty vector. this->push_back(::std::forward(Elt)); return this->end()-1; @@ -690,11 +629,14 @@ class SmallVectorImpl : public SmallVectorTemplateBase { assert(this->isReferenceToStorage(I) && "Insertion iterator is out of bounds."); - // Grow if necessary. - size_t Index = I - this->begin(); - std::remove_reference_t *EltPtr = - this->reserveForAndGetAddress(Elt); - I = this->begin() + Index; + // Check that adding an element won't invalidate Elt. + this->assertSafeToAdd(&Elt); + + if (this->size() >= this->capacity()) { + size_t EltNo = I-this->begin(); + this->grow(); + I = this->begin()+EltNo; + } ::new ((void*) this->end()) T(::std::move(this->back())); // Push everything else over. @@ -702,10 +644,9 @@ class SmallVectorImpl : public SmallVectorTemplateBase { this->set_size(this->size() + 1); // If we just moved the element we're inserting, be sure to update - // the reference (never happens if TakesParamByValue). - static_assert(!TakesParamByValue || std::is_same::value, - "ArgType must be 'T' when taking by value!"); - if (!TakesParamByValue && this->isReferenceToRange(EltPtr, I, this->end())) + // the reference. + std::remove_reference_t *EltPtr = &Elt; + if (this->isReferenceToRange(EltPtr, I, this->end())) ++EltPtr; *I = ::std::forward(*EltPtr); @@ -714,14 +655,12 @@ class SmallVectorImpl : public SmallVectorTemplateBase { public: iterator insert(iterator I, T &&Elt) { - return insert_one_impl(I, this->forward_value_param(std::move(Elt))); + return insert_one_impl(I, std::move(Elt)); } - iterator insert(iterator I, const T &Elt) { - return insert_one_impl(I, this->forward_value_param(Elt)); - } + iterator insert(iterator I, const T &Elt) { return insert_one_impl(I, Elt); } - iterator insert(iterator I, size_type NumToInsert, ValueParamT Elt) { + iterator insert(iterator I, size_type NumToInsert, const T &Elt) { // Convert iterator to elt# to avoid invalidating iterator when we reserve() size_t InsertElt = I - this->begin(); @@ -732,9 +671,11 @@ class SmallVectorImpl : public SmallVectorTemplateBase { assert(this->isReferenceToStorage(I) && "Insertion iterator is out of bounds."); - // Ensure there is enough space, and get the (maybe updated) address of - // Elt. - const T *EltPtr = this->reserveForAndGetAddress(Elt, NumToInsert); + // Check that adding NumToInsert elements won't invalidate Elt. + this->assertSafeToAdd(&Elt, NumToInsert); + + // Ensure there is enough space. + reserve(this->size() + NumToInsert); // Uninvalidate the iterator. I = this->begin()+InsertElt; @@ -751,12 +692,7 @@ class SmallVectorImpl : public SmallVectorTemplateBase { // Copy the existing elements that get replaced. std::move_backward(I, OldEnd-NumToInsert, OldEnd); - // If we just moved the element we're inserting, be sure to update - // the reference (never happens if TakesParamByValue). - if (!TakesParamByValue && I <= EltPtr && EltPtr < this->end()) - EltPtr += NumToInsert; - - std::fill_n(I, NumToInsert, *EltPtr); + std::fill_n(I, NumToInsert, Elt); return I; } @@ -769,16 +705,11 @@ class SmallVectorImpl : public SmallVectorTemplateBase { size_t NumOverwritten = OldEnd-I; this->uninitialized_move(I, OldEnd, this->end()-NumOverwritten); - // If we just moved the element we're inserting, be sure to update - // the reference (never happens if TakesParamByValue). - if (!TakesParamByValue && I <= EltPtr && EltPtr < this->end()) - EltPtr += NumToInsert; - // Replace the overwritten part. - std::fill_n(I, NumOverwritten, *EltPtr); + std::fill_n(I, NumOverwritten, Elt); // Insert the non-overwritten middle part. - std::uninitialized_fill_n(OldEnd, NumToInsert - NumOverwritten, *EltPtr); + std::uninitialized_fill_n(OldEnd, NumToInsert-NumOverwritten, Elt); return I; } diff --git a/llvm/unittests/ADT/SmallVectorTest.cpp b/llvm/unittests/ADT/SmallVectorTest.cpp index 1f6c7db99fa81a..d97ab577524f46 100644 --- a/llvm/unittests/ADT/SmallVectorTest.cpp +++ b/llvm/unittests/ADT/SmallVectorTest.cpp @@ -53,7 +53,6 @@ class Constructable { Constructable(Constructable && src) : constructed(true) { value = src.value; - src.value = 0; ++numConstructorCalls; ++numMoveConstructorCalls; } @@ -75,7 +74,6 @@ class Constructable { Constructable & operator=(Constructable && src) { EXPECT_TRUE(constructed); value = src.value; - src.value = 0; ++numAssignmentCalls; ++numMoveAssignmentCalls; return *this; @@ -1058,16 +1056,11 @@ class SmallVectorReferenceInvalidationTest : public SmallVectorTestBase { return N; } - template static bool isValueType() { - return std::is_same::value; - } - void SetUp() override { SmallVectorTestBase::SetUp(); // Fill up the small size so that insertions move the elements. - for (int I = 0, E = NumBuiltinElts(V); I != E; ++I) - V.emplace_back(I + 1); + V.append({0, 0, 0}); } }; @@ -1081,84 +1074,39 @@ TYPED_TEST_CASE(SmallVectorReferenceInvalidationTest, SmallVectorReferenceInvalidationTestTypes); TYPED_TEST(SmallVectorReferenceInvalidationTest, PushBack) { - // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode. auto &V = this->V; - int N = this->NumBuiltinElts(V); - - // Push back a reference to last element when growing from small storage. - V.push_back(V.back()); - EXPECT_EQ(N, V.back()); - - // Check that the old value is still there (not moved away). - EXPECT_EQ(N, V[V.size() - 2]); - - // Fill storage again. - V.back() = V.size(); - while (V.size() < V.capacity()) - V.push_back(V.size() + 1); - - // Push back a reference to last element when growing from large storage. - V.push_back(V.back()); - EXPECT_EQ(int(V.size()) - 1, V.back()); + (void)V; +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.push_back(V.back()), this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, PushBackMoved) { - // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode. auto &V = this->V; - int N = this->NumBuiltinElts(V); - - // Push back a reference to last element when growing from small storage. - V.push_back(std::move(V.back())); - EXPECT_EQ(N, V.back()); - if (this->template isValueType()) { - // Check that the value was moved (not copied). - EXPECT_EQ(0, V[V.size() - 2]); - } - - // Fill storage again. - V.back() = V.size(); - while (V.size() < V.capacity()) - V.push_back(V.size() + 1); - - // Push back a reference to last element when growing from large storage. - V.push_back(std::move(V.back())); - - // Check the values. - EXPECT_EQ(int(V.size()) - 1, V.back()); - if (this->template isValueType()) { - // Check the value got moved out. - EXPECT_EQ(0, V[V.size() - 2]); - } + (void)V; +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.push_back(std::move(V.back())), this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, Resize) { auto &V = this->V; (void)V; int N = this->NumBuiltinElts(V); - V.resize(N + 1, V.back()); - EXPECT_EQ(N, V.back()); - - // Resize to add enough elements that V will grow again. If reference - // invalidation breaks in the future, sanitizers should be able to catch a - // use-after-free here. - V.resize(V.capacity() + 1, V.front()); - EXPECT_EQ(1, V.back()); +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.resize(N + 1, V.back()), this->AssertionMessage); +#endif + + // No assertion when shrinking, since the parameter isn't accessed. + V.resize(N - 1, V.back()); } TYPED_TEST(SmallVectorReferenceInvalidationTest, Append) { auto &V = this->V; (void)V; - V.append(1, V.back()); - int N = this->NumBuiltinElts(V); - EXPECT_EQ(N, V[N - 1]); - - // Append enough more elements that V will grow again. This tests growing - // when already in large mode. - // - // If reference invalidation breaks in the future, sanitizers should be able - // to catch a use-after-free here. - V.append(V.capacity() - V.size() + 1, V.front()); - EXPECT_EQ(1, V.back()); +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.append(1, V.back()), this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, AppendRange) { @@ -1202,72 +1150,28 @@ TYPED_TEST(SmallVectorReferenceInvalidationTest, AssignRange) { } TYPED_TEST(SmallVectorReferenceInvalidationTest, Insert) { - // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode. auto &V = this->V; (void)V; - - // Insert a reference to the back (not at end() or else insert delegates to - // push_back()), growing out of small mode. Confirm the value was copied out - // (moving out Constructable sets it to 0). - V.insert(V.begin(), V.back()); - EXPECT_EQ(int(V.size() - 1), V.front()); - EXPECT_EQ(int(V.size() - 1), V.back()); - - // Fill up the vector again. - while (V.size() < V.capacity()) - V.push_back(V.size() + 1); - - // Grow again from large storage to large storage. - V.insert(V.begin(), V.back()); - EXPECT_EQ(int(V.size() - 1), V.front()); - EXPECT_EQ(int(V.size() - 1), V.back()); +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.insert(V.begin(), V.back()), this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, InsertMoved) { - // Note: setup adds [1, 2, ...] to V until it's at capacity in small mode. auto &V = this->V; (void)V; - - // Insert a reference to the back (not at end() or else insert delegates to - // push_back()), growing out of small mode. Confirm the value was copied out - // (moving out Constructable sets it to 0). - V.insert(V.begin(), std::move(V.back())); - EXPECT_EQ(int(V.size() - 1), V.front()); - if (this->template isValueType()) { - // Check the value got moved out. - EXPECT_EQ(0, V.back()); - } - - // Fill up the vector again. - while (V.size() < V.capacity()) - V.push_back(V.size() + 1); - - // Grow again from large storage to large storage. - V.insert(V.begin(), std::move(V.back())); - EXPECT_EQ(int(V.size() - 1), V.front()); - if (this->template isValueType()) { - // Check the value got moved out. - EXPECT_EQ(0, V.back()); - } +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.insert(V.begin(), std::move(V.back())), + this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, InsertN) { auto &V = this->V; (void)V; - - // Cover NumToInsert <= this->end() - I. - V.insert(V.begin() + 1, 1, V.back()); - int N = this->NumBuiltinElts(V); - EXPECT_EQ(N, V[1]); - - // Cover NumToInsert > this->end() - I, inserting enough elements that V will - // also grow again; V.capacity() will be more elements than necessary but - // it's a simple way to cover both conditions. - // - // If reference invalidation breaks in the future, sanitizers should be able - // to catch a use-after-free here. - V.insert(V.begin(), V.capacity(), V.front()); - EXPECT_EQ(1, V.front()); +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST + EXPECT_DEATH(V.insert(V.begin(), 2, V.back()), this->AssertionMessage); +#endif } TYPED_TEST(SmallVectorReferenceInvalidationTest, InsertRange) {