From bb217f6a925d11ed3ef8f31171dc42187b5958b5 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Mon, 19 Oct 2026 14:38:15 -0700 Subject: [PATCH 1/4] Fix VLA push_back/emplace_back of an aliased element during growth (#245) push_back(const T&), push_back(T&&) and emplace_back(Args&&...) grew the storage before reading their argument. When the argument referred to an element of the same array and the array was full, growth relocated that element and freed the old buffer first, so the new element was built from a moved-from object in released memory. All three now go through emplace_back, which on growth calls the new VariableLengthArrayBase::grow_and_emplace_back: - Without the allocator reallocate extension it allocates the new buffer, constructs the new element there while the old storage is intact, and then relocates the old elements, as libstdc++'s _M_realloc_insert does. A failure in either step destroys what was built and releases the new buffer, leaving the array unchanged. - For trivially copyable elements with a reallocating allocator, which may release the old block before anything is constructed, it builds the element in a temporary first and copies it into place after growth. Growth policy, allocation sequence, the length_error and bad_alloc behaviour, and the exceptions-disabled contract (size() unchanged when growth fails) are unchanged. VariableLengthArray takes its argument by value and is unaffected; a test confirms it. Tests check push_back and emplace_back of the first, a middle and the last element, by copy and by move, with and without reallocation, against std::vector, plus the reallocate path, allocation failure and construction failure rollback. Closes #245. Co-Authored-By: Claude Opus 5.5 --- cetlvast/suites/unittest/CMakeLists.txt | 1 + .../test_variable_length_array_push_back.cpp | 249 ++++++++++++++++++ ...est_variable_length_array_reallocation.cpp | 159 +++++++++++ include/cetl/variable_length_array.hpp | 190 ++++++++----- 4 files changed, 530 insertions(+), 69 deletions(-) create mode 100644 cetlvast/suites/unittest/test_variable_length_array_push_back.cpp diff --git a/cetlvast/suites/unittest/CMakeLists.txt b/cetlvast/suites/unittest/CMakeLists.txt index c2a0c58d..1bb55540 100644 --- a/cetlvast/suites/unittest/CMakeLists.txt +++ b/cetlvast/suites/unittest/CMakeLists.txt @@ -123,6 +123,7 @@ set(NATIVE_TESTS test_variable_length_array_general_allocation.cpp test_variable_length_array_insert.cpp test_variable_length_array_move_capacity.cpp + test_variable_length_array_push_back.cpp test_variable_length_array_exception_safety.cpp test_variable_length_array_reallocation.cpp test_variable_length_array_reverse_iterators.cpp diff --git a/cetlvast/suites/unittest/test_variable_length_array_push_back.cpp b/cetlvast/suites/unittest/test_variable_length_array_push_back.cpp new file mode 100644 index 00000000..5920529b --- /dev/null +++ b/cetlvast/suites/unittest/test_variable_length_array_push_back.cpp @@ -0,0 +1,249 @@ +/// @file +/// push_back and emplace_back of an argument that refers to an element of the same array (issue #245), checked +/// against std::vector with and without reallocation. +/// @copyright +/// Copyright (C) OpenCyphal Development Team +/// SPDX-License-Identifier: MIT + +#include "cetl/variable_length_array.hpp" +#include "cetlvast/helpers_gtest.hpp" + +#include +#include +#include +#include +#include + +namespace +{ +template +using Array = cetl::VariableLengthArray>; + +constexpr std::size_t ElementCount = 5; + +// Long enough to need its own heap allocation, so a read from a moved-from or released element is visible. +std::string string_value(const std::size_t index) +{ + return std::string(64U, static_cast('a' + index)) + std::to_string(index); +} + +int int_value(const std::size_t index) +{ + return static_cast(100U + (index * 7U)); +} + +bool bool_value(const std::size_t index) +{ + return (index % 3U) != 1U; +} + +std::string make(const std::size_t index, const std::string*) +{ + return string_value(index); +} + +int make(const std::size_t index, const int*) +{ + return int_value(index); +} + +bool make(const std::size_t index, const bool*) +{ + return bool_value(index); +} + +// The ways to append a value that the arguments derive from element `at`. +enum class Append +{ + PushBackCopy, + PushBackMove, + EmplaceBackCopy, + EmplaceBackMove, +}; + +const char* to_string(const Append append) +{ + switch (append) + { + case Append::PushBackCopy: + return "push_back(const T&)"; + case Append::PushBackMove: + return "push_back(T&&)"; + case Append::EmplaceBackCopy: + return "emplace_back(T&)"; + case Append::EmplaceBackMove: + return "emplace_back(T&&)"; + } + return "?"; // GCOVR_EXCL_LINE +} + +template +void append(Container& container, const Append how, const std::size_t at) +{ + switch (how) + { + case Append::PushBackCopy: + container.push_back(static_cast(container[at])); + break; + case Append::PushBackMove: + container.push_back(std::move(container[at])); + break; + case Append::EmplaceBackCopy: + container.emplace_back(container[at]); + break; + case Append::EmplaceBackMove: + container.emplace_back(std::move(container[at])); + break; + } +} + +bool moves(const Append how) +{ + return (how == Append::PushBackMove) || (how == Append::EmplaceBackMove); +} + +template +class VLAAppendElement : public ::testing::Test +{}; +using ElementTypes = ::testing::Types; +TYPED_TEST_SUITE(VLAAppendElement, ElementTypes, ); + +// The first, a middle, and the last element, appended by every overload, with the array full (so the append must +// reallocate) and with spare capacity. A moved-from element has an unspecified value, so it is not compared. +TYPED_TEST(VLAAppendElement, MatchesVector) +{ + const TypeParam* const tag = nullptr; + for (const Append how : + {Append::PushBackCopy, Append::PushBackMove, Append::EmplaceBackCopy, Append::EmplaceBackMove}) + { + for (const std::size_t at : {std::size_t{0}, ElementCount / 2U, ElementCount - 1U}) + { + for (const bool reallocates : {true, false}) + { + SCOPED_TRACE(::testing::Message() << to_string(how) << " at=" << at << " reallocates=" << reallocates); + Array actual{std::allocator{}}; + std::vector expected; + actual.reserve(reallocates ? ElementCount : (ElementCount + 1U)); + for (std::size_t i = 0; i < ElementCount; ++i) + { + actual.push_back(make(i, tag)); + expected.push_back(make(i, tag)); + } + ASSERT_EQ(actual.capacity() == actual.size(), reallocates); + const TypeParam* const data_before = actual.data(); + + append(actual, how, at); + append(expected, how, at); + + EXPECT_EQ(actual.data() != data_before, reallocates); + ASSERT_EQ(actual.size(), expected.size()); + for (std::size_t i = 0; i < actual.size(); ++i) + { + if (!moves(how) || (i != at)) + { + EXPECT_EQ(actual[i], expected[i]) << "index=" << i; + } + } + EXPECT_EQ(actual.back(), make(at, tag)); + } + } + } +} + +// emplace_back arguments other than a whole element may also refer into the array, here a substring of one. +TEST(VLAAppendElement, EmplaceBackConstructorArgumentsMatchVector) +{ + for (const bool reallocates : {true, false}) + { + SCOPED_TRACE(::testing::Message() << "reallocates=" << reallocates); + Array actual{std::allocator{}}; + std::vector expected; + actual.reserve(reallocates ? ElementCount : (ElementCount + 1U)); + for (std::size_t i = 0; i < ElementCount; ++i) + { + actual.push_back(string_value(i)); + expected.push_back(string_value(i)); + } + const std::string* const data_before = actual.data(); + + actual.emplace_back(actual[1], 2U, 40U); + expected.emplace_back(expected[1], 2U, 40U); + + EXPECT_EQ(actual.data() != data_before, reallocates); + ASSERT_EQ(actual.size(), expected.size()); + for (std::size_t i = 0; i < actual.size(); ++i) + { + EXPECT_EQ(actual[i], expected[i]) << "index=" << i; + } + EXPECT_EQ(actual.back(), string_value(1).substr(2U, 40U)); + } +} + +// Repeatedly appending the first and last elements grows through several capacities. +TEST(VLAAppendElement, RepeatedAppendsAcrossGrowthMatchVector) +{ + Array actual{std::allocator{}}; + std::vector expected; + actual.push_back(string_value(0)); + expected.push_back(string_value(0)); + for (std::size_t i = 0; i < 40U; ++i) + { + if ((i % 2U) == 0U) + { + actual.push_back(actual.front()); + expected.push_back(expected.front()); + } + else + { + actual.emplace_back(actual.back() + "+"); + expected.emplace_back(expected.back() + "+"); + } + } + ASSERT_EQ(actual.size(), expected.size()); + for (std::size_t i = 0; i < actual.size(); ++i) + { + EXPECT_EQ(actual[i], expected[i]) << "index=" << i; + } +} + +// The bool specialization takes its argument by value, so the bit is read before the storage can change. +TEST(VLAAppendElement, BoolMatchesVector) +{ + const bool* const tag = nullptr; + for (const std::size_t at : {std::size_t{0}, ElementCount / 2U, ElementCount - 1U}) + { + for (const bool emplace : {false, true}) + { + SCOPED_TRACE(::testing::Message() << "at=" << at << " emplace=" << emplace); + Array actual{std::allocator{}}; + std::vector expected; + for (std::size_t i = 0; i < ElementCount; ++i) + { + actual.push_back(make(i, tag)); + expected.push_back(make(i, tag)); + } + // One storage byte holds eight bits, so the ninth append grows the storage. + actual.shrink_to_fit(); + ASSERT_EQ(actual.capacity(), 8U); + for (std::size_t i = ElementCount; i < 9U; ++i) + { + if (emplace) + { + actual.emplace_back(actual[at]); + expected.emplace_back(expected[at]); + } + else + { + actual.push_back(actual[at]); + expected.push_back(expected[at]); + } + } + ASSERT_EQ(actual.size(), expected.size()); + for (std::size_t i = 0; i < actual.size(); ++i) + { + EXPECT_EQ(actual[i], expected[i]) << "index=" << i; + } + } + } +} +} // namespace diff --git a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp index 6e087b26..56dc94e2 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp @@ -14,6 +14,7 @@ #include #include #include +#include namespace { @@ -641,4 +642,162 @@ TEST(VLAReallocationGrowth, AppendAndResizePreserveExistingElements) EXPECT_EQ(array[i].self, &array[i].value); } } + +int value_of(const int element) noexcept +{ + return element; +} + +int value_of(const SelfReference& element) noexcept +{ + EXPECT_EQ(element.self, &element.value); + return element.value; +} + +// Appending an element of a full array must read it before the old storage is released (issue #245). Trivially +// copyable elements grow through reallocate, or allocate when it is declined; other elements always allocate. +template +void expect_appended_elements_survive_growth(const bool decline) +{ + MovingResource resource; + resource.decline = decline; + Array array{typename Array::allocator_type{&resource}}; + array.reserve(AppendInitialCapacity); + for (std::size_t i = 0; i < AppendInitialCapacity; ++i) + { + array.emplace_back(element_value(i)); + } + std::vector expected; + for (const auto& element : array) + { + expected.push_back(value_of(element)); + } + // Each round fills the spare capacity, then appends the first element, then the last, each time from full storage. + for (std::size_t round = 0; round < 3; ++round) + { + for (const bool emplace : {false, true}) + { + while (array.size() < array.capacity()) + { + array.emplace_back(element_value(array.size())); + expected.push_back(element_value(expected.size())); + } + const T* const data_before = array.data(); + const auto reallocations = resource.reallocations; + if (emplace) + { + array.emplace_back(array.back()); + expected.push_back(expected.back()); + } + else + { + array.push_back(static_cast(array.front())); + expected.push_back(expected.front()); + } + EXPECT_NE(array.data(), data_before); + EXPECT_EQ(resource.reallocations, reallocations + (std::is_trivially_copyable::value ? 1U : 0U)); + } + } + ASSERT_EQ(array.size(), expected.size()); + for (std::size_t i = 0; i < array.size(); ++i) + { + EXPECT_EQ(value_of(array[i]), expected[i]) << "index=" << i; + } + EXPECT_EQ(resource.blocks.size(), 1U); +} + +TEST(VLAReallocationGrowth, AppendedElementsSurviveGrowth) +{ + for (const bool decline : {false, true}) + { + SCOPED_TRACE(decline); + expect_appended_elements_survive_growth(decline); + expect_appended_elements_survive_growth(decline); + } +} + +// An allocator out of memory makes appending to a full array throw bad_alloc or, with exceptions disabled, return +// without effect. Either way the original storage and elements remain. +template +void expect_failed_append_preserves_storage() +{ + MovingResource resource; + Array array{typename Array::allocator_type{&resource}}; + array.reserve(AppendInitialCapacity); + for (std::size_t i = 0; i < AppendInitialCapacity; ++i) + { + array.emplace_back(element_value(i)); + } + const T* const data_before = array.data(); + resource.fail = true; +#if defined(__cpp_exceptions) + EXPECT_THROW(array.push_back(static_cast(array.front())), std::bad_alloc); + EXPECT_THROW(array.emplace_back(array.back()), std::bad_alloc); +#else + array.push_back(static_cast(array.front())); + array.emplace_back(array.back()); +#endif + EXPECT_EQ(array.data(), data_before); + EXPECT_EQ(array.capacity(), AppendInitialCapacity); + ASSERT_EQ(array.size(), AppendInitialCapacity); + for (std::size_t i = 0; i < array.size(); ++i) + { + EXPECT_EQ(value_of(array[i]), element_value(i)); + } + resource.fail = false; + array.emplace_back(array.front()); + ASSERT_EQ(array.size(), AppendInitialCapacity + 1U); + EXPECT_EQ(value_of(array.back()), element_value(0)); +} + +TEST(VLAReallocationGrowth, FailedAllocationOnAppendPreservesStorage) +{ + expect_failed_append_preserves_storage(); + expect_failed_append_preserves_storage(); +} + +#if defined(__cpp_exceptions) +// The appended element is constructed first and the old elements are then copied around it. A failure at any of +// those constructions must destroy what was built, release the replacement storage, and keep the original elements. +TEST(VLAReallocationGrowth, FailedAppendRollsBackReplacement) +{ + for (std::size_t fail_at = 0; fail_at <= ElementCount; ++fail_at) + { + SCOPED_TRACE(fail_at); + MovingResource resource; + Lifetimes state; + Array array{typename Array::allocator_type{&resource}}; + array.reserve(ElementCount); + for (std::size_t i = 0; i < ElementCount; ++i) + { + array.emplace_back(state, element_value(i)); + } + const auto* const old_data = array.data(); + state.remaining = static_cast(fail_at); + EXPECT_THROW(array.push_back(static_cast(array.front())), ConstructionFailure); + EXPECT_EQ(array.data(), old_data); + EXPECT_EQ(array.capacity(), ElementCount); + ASSERT_EQ(array.size(), ElementCount); + EXPECT_EQ(resource.blocks.size(), 1U); + EXPECT_EQ(state.constructed, ElementCount + fail_at); + EXPECT_EQ(state.destroyed, fail_at); + EXPECT_EQ(state.live.size(), ElementCount); + EXPECT_EQ(state.moves, 0U); + for (std::size_t i = 0; i < array.size(); ++i) + { + EXPECT_EQ(array[i].value, element_value(i)); + EXPECT_EQ(array[i].self, &array[i].value); + } + state.remaining = FailureDisabled; + array.emplace_back(array.front()); + ASSERT_EQ(array.size(), ElementCount + 1U); + for (std::size_t i = 0; i < array.size(); ++i) + { + EXPECT_EQ(array[i].value, element_value(i < ElementCount ? i : 0U)); + EXPECT_EQ(array[i].self, &array[i].value); + } + EXPECT_EQ(resource.blocks.size(), 1U); + } +} +#endif } // namespace diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index 474d0f52..66f24c31 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -1047,23 +1047,123 @@ class VariableLengthArrayBase } } - /// Grows the capacity of the array ensures amortized, constant-time expansion. - constexpr bool grow(const size_type max_size) + /// The capacity that grow() requests. It is not greater than capacity() when max_size prevents growth. + constexpr size_type grown_capacity(const size_type max_size) const noexcept { // Simple geometric progression of capacity growth. - const size_type capacity_before = capacity_; - - const size_type half_capacity = capacity_before / 2U; + const size_type half_capacity = capacity_ / 2U; // That is, instead of a capacity sequence of 1, 2, 3, 4, 6, 9 we start from zero as 2, 4, 6, 9. The first // opportunity for reusing previously freed memory comes when increasing to 19 from 13 since E(n-1) == 21. - const size_type new_capacity = std::min(max_size, capacity_before + ((half_capacity <= 1) ? 2 : half_capacity)); + return std::min(max_size, capacity_ + ((half_capacity <= 1) ? 2 : half_capacity)); + } + + /// Grows the capacity of the array ensures amortized, constant-time expansion. + constexpr bool grow(const size_type max_size) + { + const size_type capacity_before = capacity_; - reserve(new_capacity, max_size); + reserve(grown_capacity(max_size), max_size); return capacity_ > capacity_before; } + /// Grows the capacity as grow() does and constructs one more element from `args` at the end of the array. + /// The arguments may refer to elements of this array, as in `a.emplace_back(a[0])`, so they are consumed before + /// the old storage is released. + /// @return false, with the array unchanged, if max_size prevents growth or, with exceptions disabled, the + /// allocator returned no memory. + /// @throw * Any exception thrown by the allocator or by constructing an element. The array is then unchanged. + template + bool grow_and_emplace_back(const size_type max_size, Args&&... args) + { + if (grown_capacity(max_size) <= capacity_) + { + return false; + } + return grow_and_emplace_back_impl(reallocates_elements{}, max_size, std::forward(args)...); + } + + /// Whether reserve() may relocate elements through the allocator's reallocate extension (see reallocate()). + using reallocates_elements = std::integral_constant::value && + is_detected::value>; + + /// Allocator reallocation can release the old block before an element is constructed in the new one, so the new + /// element is built in a temporary first. It is trivially copyable, so moving it into place is a plain copy. + template + bool grow_and_emplace_back_impl(std::true_type, const size_type max_size, Args&&... args) + { + // The union defers construction to the allocator, which might observe it, as it would any element. + union Temporary + { + Temporary() noexcept {} + ~Temporary() {} + value_type value; + } temporary; + value_type* const pending = std::addressof(temporary.value); + std::allocator_traits::construct(alloc_, pending, std::forward(args)...); + // Trivially copyable types are trivially destructible, so an exception from grow() leaks nothing. + const bool grown = grow(max_size); + if (grown) + { + std::allocator_traits::construct(alloc_, data_ + size_, std::move(*pending)); + ++size_; + } + std::allocator_traits::destroy(alloc_, pending); + return grown; + } + + /// Constructs the new element in replacement storage while the old storage is intact, then relocates the old + /// elements in front of it, as libstdc++'s `vector::_M_realloc_insert` does. + template + bool grow_and_emplace_back_impl(std::false_type, const size_type max_size, Args&&... args) + { + const size_type new_capacity = grown_capacity(max_size); + value_type* const new_data = std::allocator_traits::allocate(alloc_, new_capacity); + if (new_data == nullptr) + { +#if defined(__cpp_exceptions) + // GCOVR_EXCL_START + // protect against invalid standard libraries that do not throw as required. + throw std::bad_alloc(); + // GCOVR_EXCL_STOP +#else + return false; +#endif + } + value_type* const element = new_data + size_; +#if defined(__cpp_exceptions) + try + { +#endif + std::allocator_traits::construct(alloc_, element, std::forward(args)...); +#if defined(__cpp_exceptions) + } catch (...) + { + std::allocator_traits::deallocate(alloc_, new_data, new_capacity); + throw; + } + try + { +#endif + // On failure this destroys the elements it relocated, leaving the old ones valid. + (void) fast_forward_construct(new_data, new_capacity, data_, size_, alloc_); +#if defined(__cpp_exceptions) + } catch (...) + { + std::allocator_traits::destroy(alloc_, element); + std::allocator_traits::deallocate(alloc_, new_data, new_capacity); + throw; + } +#endif + fast_deallocate(data_, size_, capacity_, alloc_); + data_ = new_data; + capacity_ = new_capacity; + ++size_; + return true; + } + // +----------------------------------------------------------------------+ // | CONSTRUCTORS // +----------------------------------------------------------------------+ @@ -1922,12 +2022,7 @@ class VariableLengthArray : protected detail::VariableLengthArrayBase(value)); + emplace_back(std::move(value)); } /// @@ -2075,20 +2165,26 @@ class VariableLengthArray : protected detail::VariableLengthArrayBase void emplace_back(Args&&... args) { - if (!ensure_size_plus_one()) + // Spare capacity can exceed max_size() after assignment adopts a smaller bound, so check both. + if ((size_ < capacity_) && (size_ < max_size())) { - return; + std::allocator_traits::construct(alloc_, data_ + size_, std::forward(args)...); + ++size_; + } + else if (!Base::grow_and_emplace_back(max_size(), std::forward(args)...)) + { +#if defined(__cpp_exceptions) + throw std::length_error("size is at capacity and we cannot grow the capacity."); +#endif } - - std::allocator_traits::construct(alloc_, &data_[size_], std::forward(args)...); - size_++; } /// Resizes internal storage to count elements default initializing any added elements over the @@ -2340,50 +2436,6 @@ class VariableLengthArray : protected detail::VariableLengthArrayBase::value) - { - CETL_DEBUG_ASSERT(size_ < capacity_, "CDE_vla_008: No capacity to push element."); - if (size_ < capacity_) - { - std::allocator_traits::construct(alloc_, &data_[size_], std::move(value)); - size_++; - } - } - - constexpr pointer push_back_impl(const value_type& value) noexcept( - std::is_nothrow_copy_constructible::value) - { - if (size_ < capacity_) - { - std::allocator_traits::construct(alloc_, &data_[size_], value); - return &data_[size_++]; - } - else - { - return nullptr; - } - } }; // +-------------------------------------------------------------------------------------------------------------------+ From 82f404fd013b139501c34fb64880320696dc1b25 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Mon, 19 Oct 2026 16:39:14 -0700 Subject: [PATCH 2/4] Byte-relocate the appended element on the VLA reallocate path On the reallocate path (trivially copyable elements, allocator with reallocate), grow_and_emplace_back built the new element in a stack temporary and then move-constructed it into place through the allocator. That needed a move constructor, so trivially copyable types with a deleted move constructor stopped compiling, and the allocator's construct hook ran twice for one append (plus a destroy for the temporary). The temporary is now relocated by copying its bytes, as reallocate() already does for the other elements. The allocator constructs the new element once and needs no move constructor, and it destroys the temporary only when the append fails. Tests add a copy-only trivially copyable element to the growth and allocation-failure tests, and an allocator that counts construct and destroy calls. Addresses Codex review comments on #247. Co-Authored-By: Claude Opus 5.5 --- ...est_variable_length_array_reallocation.cpp | 118 ++++++++++++++++++ include/cetl/variable_length_array.hpp | 30 +++-- 2 files changed, 140 insertions(+), 8 deletions(-) diff --git a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp index 56dc94e2..a2861a76 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp @@ -654,6 +654,28 @@ int value_of(const SelfReference& element) noexcept return element.value; } +// Trivially copyable but not movable, so relocating an appended element must not need a move constructor. +struct CopyOnly +{ + int value; + + explicit CopyOnly(int v) noexcept + : value(v) + { + } + CopyOnly(const CopyOnly&) = default; + CopyOnly(CopyOnly&&) = delete; + CopyOnly& operator=(const CopyOnly&) = default; + CopyOnly& operator=(CopyOnly&&) = delete; + ~CopyOnly() = default; +}; +static_assert(!std::is_move_constructible::value, "Exercise growth without a move constructor."); + +int value_of(const CopyOnly& element) noexcept +{ + return element.value; +} + // Appending an element of a full array must read it before the old storage is released (issue #245). Trivially // copyable elements grow through reallocate, or allocate when it is declined; other elements always allocate. template @@ -712,6 +734,7 @@ TEST(VLAReallocationGrowth, AppendedElementsSurviveGrowth) { SCOPED_TRACE(decline); expect_appended_elements_survive_growth(decline); + expect_appended_elements_survive_growth(decline); expect_appended_elements_survive_growth(decline); } } @@ -753,9 +776,104 @@ void expect_failed_append_preserves_storage() TEST(VLAReallocationGrowth, FailedAllocationOnAppendPreservesStorage) { expect_failed_append_preserves_storage(); + expect_failed_append_preserves_storage(); expect_failed_append_preserves_storage(); } +struct HookCounts +{ + std::size_t constructs = 0; + std::size_t destroys = 0; +}; + +// A reallocate-capable allocator that counts its construct and destroy hooks. +template +class HookCountingAllocator : public cetl::pf17::pmr::polymorphic_allocator +{ +public: + using Base = cetl::pf17::pmr::polymorphic_allocator; + + HookCountingAllocator(cetl::pf17::pmr::memory_resource* resource, HookCounts* counts) noexcept + : Base(resource) + , counts_(counts) + { + } + + template + HookCountingAllocator(const HookCountingAllocator& other) noexcept // NOLINT(*-explicit-constructor) + : Base(other.resource()) + , counts_(other.counts()) + { + } + + template + void construct(U* pointer, Args&&... args) + { + ++counts_->constructs; + Base::construct(pointer, std::forward(args)...); + } + + template + void destroy(U* pointer) + { + ++counts_->destroys; + pointer->~U(); + } + + HookCounts* counts() const noexcept + { + return counts_; + } + +private: + HookCounts* counts_; +}; + +// The temporary that keeps an aliased argument alive through reallocate is the element itself: the allocator constructs +// it once, and destroys it only when the append fails. +TEST(VLAReallocationGrowth, AppendConstructsOnceThroughAllocator) +{ + for (const bool decline : {false, true}) + { + SCOPED_TRACE(decline); + MovingResource resource; + resource.decline = decline; + HookCounts counts; + cetl::VariableLengthArray> array{ + HookCountingAllocator{&resource, &counts}}; + array.reserve(AppendInitialCapacity); + for (std::size_t i = 0; i < AppendInitialCapacity; ++i) + { + array.push_back(element_value(i)); + } + ASSERT_EQ(counts.constructs, AppendInitialCapacity); + const auto reallocations = resource.reallocations; + array.push_back(static_cast(array.front())); + EXPECT_EQ(resource.reallocations, reallocations + 1U); + // When reallocate is declined, the fallback also relocates each old element through the allocator. + EXPECT_EQ(counts.constructs, AppendInitialCapacity + 1U + (decline ? AppendInitialCapacity : 0U)); + EXPECT_EQ(counts.destroys, 0U); + EXPECT_EQ(array.back(), element_value(0)); + + while (array.size() < array.capacity()) + { + array.push_back(element_value(array.size())); + } + const auto constructs = counts.constructs; + const auto size = array.size(); + resource.fail = true; +#if defined(__cpp_exceptions) + EXPECT_THROW(array.emplace_back(array.back()), std::bad_alloc); +#else + array.emplace_back(array.back()); +#endif + resource.fail = false; + EXPECT_EQ(array.size(), size); + EXPECT_EQ(counts.constructs, constructs + 1U); + EXPECT_EQ(counts.destroys, 1U); + } +} + #if defined(__cpp_exceptions) // The appended element is constructed first and the old elements are then copied around it. A failure at any of // those constructions must destroy what was built, release the replacement storage, and keep the original elements. diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index 66f24c31..a887c580 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -1090,7 +1090,8 @@ class VariableLengthArrayBase is_detected::value>; /// Allocator reallocation can release the old block before an element is constructed in the new one, so the new - /// element is built in a temporary first. It is trivially copyable, so moving it into place is a plain copy. + /// element is constructed in a temporary first. Like the elements that reallocate() moves, it is then relocated by + /// copying its bytes, which constructs it through the allocator only once and needs no move constructor. template bool grow_and_emplace_back_impl(std::true_type, const size_type max_size, Args&&... args) { @@ -1103,15 +1104,28 @@ class VariableLengthArrayBase } temporary; value_type* const pending = std::addressof(temporary.value); std::allocator_traits::construct(alloc_, pending, std::forward(args)...); - // Trivially copyable types are trivially destructible, so an exception from grow() leaks nothing. - const bool grown = grow(max_size); - if (grown) + bool grown = false; +#if defined(__cpp_exceptions) + try { - std::allocator_traits::construct(alloc_, data_ + size_, std::move(*pending)); - ++size_; +#endif + grown = grow(max_size); +#if defined(__cpp_exceptions) + } catch (...) + { + std::allocator_traits::destroy(alloc_, pending); + throw; } - std::allocator_traits::destroy(alloc_, pending); - return grown; +#endif + if (!grown) + { + std::allocator_traits::destroy(alloc_, pending); + return false; + } + // The relocated bytes are now the element, so the temporary is not destroyed. + (void) std::memcpy(static_cast(data_ + size_), static_cast(pending), sizeof(value_type)); + ++size_; + return true; } /// Constructs the new element in replacement storage while the old storage is intact, then relocates the old From dceb304ec0c262584e26952c57573dbc62036d29 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Mon, 19 Oct 2026 16:58:11 -0700 Subject: [PATCH 3/4] Construct appended VLA elements in their final storage On the reallocate path, the appended element was constructed in a stack temporary and its bytes copied into the array afterwards. The allocator's construct hook therefore saw a stack address rather than the element, and a constructor that records `this` was left pointing at the expired temporary. The element is now always constructed once, through the allocator, at its final address. On the reallocate path the code depends on the arguments: - No arguments: grow as usual, then construct in place. - One argument of the element type (every push_back, emplace_back(a[i])): if it is not an element of this array, grow, then construct in place, with no temporary at all. If it is, first copy its bytes, which runs no constructor, then construct in place from the copy with the argument's value category. - Other constructor arguments may refer anywhere, so construct the element in new storage before relocating the old elements, as the non-reallocating path does. Only this case gives up reallocate. refers_to_element() moves from VariableLengthArray to its base so that insert() and append share it. Tests check that each append constructs exactly once at &back(), with and without aliasing, and that a failed append constructs nothing. Also that a constructor recording `this` sees its final address, and that emplace_back() without arguments still grows through reallocate. Addresses a Codex review comment on #247. Co-Authored-By: Claude Opus 5.5 --- ...est_variable_length_array_reallocation.cpp | 144 +++++++++++++----- include/cetl/variable_length_array.hpp | 89 ++++++----- 2 files changed, 161 insertions(+), 72 deletions(-) diff --git a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp index a2861a76..761c9e58 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp @@ -782,8 +782,9 @@ TEST(VLAReallocationGrowth, FailedAllocationOnAppendPreservesStorage) struct HookCounts { - std::size_t constructs = 0; - std::size_t destroys = 0; + std::size_t constructs = 0; + std::size_t destroys = 0; + const void* last_constructed = nullptr; }; // A reallocate-capable allocator that counts its construct and destroy hooks. @@ -810,6 +811,7 @@ class HookCountingAllocator : public cetl::pf17::pmr::polymorphic_allocator void construct(U* pointer, Args&&... args) { ++counts_->constructs; + counts_->last_constructed = pointer; Base::construct(pointer, std::forward(args)...); } @@ -829,48 +831,116 @@ class HookCountingAllocator : public cetl::pf17::pmr::polymorphic_allocator HookCounts* counts_; }; -// The temporary that keeps an aliased argument alive through reallocate is the element itself: the allocator constructs -// it once, and destroys it only when the append fails. -TEST(VLAReallocationGrowth, AppendConstructsOnceThroughAllocator) +// Each append constructs its element once, through the allocator, at its final address, whether or not the argument +// is an element that reallocate releases. A failed append constructs nothing. +TEST(VLAReallocationGrowth, AppendConstructsOnceInPlaceThroughAllocator) { for (const bool decline : {false, true}) { - SCOPED_TRACE(decline); - MovingResource resource; - resource.decline = decline; - HookCounts counts; - cetl::VariableLengthArray> array{ - HookCountingAllocator{&resource, &counts}}; - array.reserve(AppendInitialCapacity); - for (std::size_t i = 0; i < AppendInitialCapacity; ++i) - { - array.push_back(element_value(i)); - } - ASSERT_EQ(counts.constructs, AppendInitialCapacity); - const auto reallocations = resource.reallocations; - array.push_back(static_cast(array.front())); - EXPECT_EQ(resource.reallocations, reallocations + 1U); - // When reallocate is declined, the fallback also relocates each old element through the allocator. - EXPECT_EQ(counts.constructs, AppendInitialCapacity + 1U + (decline ? AppendInitialCapacity : 0U)); - EXPECT_EQ(counts.destroys, 0U); - EXPECT_EQ(array.back(), element_value(0)); - - while (array.size() < array.capacity()) + for (const bool aliased : {false, true}) { - array.push_back(element_value(array.size())); - } - const auto constructs = counts.constructs; - const auto size = array.size(); - resource.fail = true; + SCOPED_TRACE(::testing::Message() << "decline=" << decline << " aliased=" << aliased); + MovingResource resource; + resource.decline = decline; + HookCounts counts; + cetl::VariableLengthArray> array{ + HookCountingAllocator{&resource, &counts}}; + array.reserve(AppendInitialCapacity); + for (std::size_t i = 0; i < AppendInitialCapacity; ++i) + { + array.push_back(element_value(i)); + } + ASSERT_EQ(counts.constructs, AppendInitialCapacity); + const int value = aliased ? array.front() : element_value(AppendInitialCapacity); + const auto reallocations = resource.reallocations; + if (aliased) + { + array.push_back(static_cast(array.front())); + } + else + { + array.push_back(value); + } + EXPECT_EQ(resource.reallocations, reallocations + 1U); + // When reallocate is declined, the fallback also relocates each old element through the allocator. + EXPECT_EQ(counts.constructs, AppendInitialCapacity + 1U + (decline ? AppendInitialCapacity : 0U)); + EXPECT_EQ(counts.last_constructed, &array.back()); + EXPECT_EQ(counts.destroys, 0U); + EXPECT_EQ(array.back(), value); + + while (array.size() < array.capacity()) + { + array.push_back(element_value(array.size())); + } + const auto constructs = counts.constructs; + const auto size = array.size(); + resource.fail = true; #if defined(__cpp_exceptions) - EXPECT_THROW(array.emplace_back(array.back()), std::bad_alloc); + if (aliased) + { + EXPECT_THROW(array.emplace_back(array.back()), std::bad_alloc); + } + else + { + EXPECT_THROW(array.emplace_back(value), std::bad_alloc); + } #else - array.emplace_back(array.back()); + if (aliased) + { + array.emplace_back(array.back()); + } + else + { + array.emplace_back(value); + } #endif - resource.fail = false; - EXPECT_EQ(array.size(), size); - EXPECT_EQ(counts.constructs, constructs + 1U); - EXPECT_EQ(counts.destroys, 1U); + resource.fail = false; + EXPECT_EQ(array.size(), size); + EXPECT_EQ(counts.constructs, constructs); + EXPECT_EQ(counts.destroys, 0U); + } + } +} + +// Trivially copyable, but its converting constructor records where it ran, as a constructor that stores `this` does. +struct Located +{ + const void* where; + int value; + + explicit Located(int v) noexcept + : where(this) + , value(v) + { + } +}; +static_assert(std::is_trivially_copyable::value, "Exercise the reallocate path."); + +// Constructor arguments other than an element may refer anywhere, so the element is constructed in its new storage +// rather than in a temporary that is then moved there. An append without arguments needs no such care. +TEST(VLAReallocationGrowth, EmplaceConstructsInFinalStorage) +{ + MovingResource resource; + Array array{typename Array::allocator_type{&resource}}; + for (std::size_t i = 0; i < AppendedElementCount * 4U; ++i) + { + const bool full = array.size() == array.capacity(); + array.emplace_back(element_value(i)); + ASSERT_EQ(array.size(), i + 1U); + EXPECT_EQ(array.back().where, &array.back()) << "full=" << full; + EXPECT_EQ(array.back().value, element_value(i)); + } + + Array defaults{typename Array::allocator_type{&resource}}; + defaults.reserve(AppendInitialCapacity); + defaults.resize(AppendInitialCapacity); + const auto reallocations = resource.reallocations; + defaults.emplace_back(); + EXPECT_EQ(resource.reallocations, reallocations + 1U); + ASSERT_EQ(defaults.size(), AppendInitialCapacity + 1U); + for (const auto& element : defaults) + { + EXPECT_EQ(element.value, FillValue); } } diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index a887c580..30479eec 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -1089,41 +1089,68 @@ class VariableLengthArrayBase std::is_trivially_copyable::value && is_detected::value>; - /// Allocator reallocation can release the old block before an element is constructed in the new one, so the new - /// element is constructed in a temporary first. Like the elements that reallocate() moves, it is then relocated by - /// copying its bytes, which constructs it through the allocator only once and needs no move constructor. + /// Whether `value` is one of the elements in [data(), data() + size()). + bool refers_to_element(const value_type& value) const noexcept + { + // std::less orders pointers that are not into the same array, which the built-in operators do not. + const std::less less; + const value_type* address = std::addressof(value); + return (data_ != nullptr) && !less(address, data_) && less(address, data_ + size_); + } + + /// Allocator reallocation can release the old block before the new element is constructed, and arbitrary + /// constructor arguments may refer into it. Construct the element in new storage before relocating the old ones + /// instead, forgoing reallocation for this append. template bool grow_and_emplace_back_impl(std::true_type, const size_type max_size, Args&&... args) { - // The union defers construction to the allocator, which might observe it, as it would any element. - union Temporary - { - Temporary() noexcept {} - ~Temporary() {} - value_type value; - } temporary; - value_type* const pending = std::addressof(temporary.value); - std::allocator_traits::construct(alloc_, pending, std::forward(args)...); - bool grown = false; -#if defined(__cpp_exceptions) - try + return grow_and_emplace_back_impl(std::false_type{}, max_size, std::forward(args)...); + } + + /// Without arguments nothing can refer into the old block, so grow as usual and construct the element in place. + bool grow_and_emplace_back_impl(std::true_type, const size_type max_size) + { + if (!grow(max_size)) { -#endif - grown = grow(max_size); -#if defined(__cpp_exceptions) - } catch (...) + return false; + } + std::allocator_traits::construct(alloc_, data_ + size_); + ++size_; + return true; + } + + /// A single element-typed argument, as from push_back(), can be checked for aliasing. Either way the element is + /// constructed once, through the allocator, in its final storage. + template , value_type>::value, int> = 0> + bool grow_and_emplace_back_impl(std::true_type, const size_type max_size, Arg&& arg) + { + if (!refers_to_element(arg)) { - std::allocator_traits::destroy(alloc_, pending); - throw; + if (!grow(max_size)) + { + return false; + } + std::allocator_traits::construct(alloc_, data_ + size_, std::forward(arg)); + ++size_; + return true; } -#endif - if (!grown) + // The argument is an element that reallocation may release. Copying the bytes of a trivially copyable object + // copies its value without running a constructor, so the copy stands in for the argument. + union Snapshot + { + Snapshot() noexcept {} + ~Snapshot() {} + value_type value; + } snapshot; + (void) std::memcpy(static_cast(std::addressof(snapshot.value)), + static_cast(std::addressof(arg)), + sizeof(value_type)); + if (!grow(max_size)) { - std::allocator_traits::destroy(alloc_, pending); return false; } - // The relocated bytes are now the element, so the temporary is not destroyed. - (void) std::memcpy(static_cast(data_ + size_), static_cast(pending), sizeof(value_type)); + // Pass the copy with the argument's value category so that the same constructor is selected. + std::allocator_traits::construct(alloc_, data_ + size_, static_cast(snapshot.value)); ++size_; return true; } @@ -2145,7 +2172,7 @@ class VariableLengthArray : protected detail::VariableLengthArrayBase less; - const value_type* address = std::addressof(value); - return (data_ != nullptr) && !less(address, data_) && less(address, data_ + size_); - } - // Makes room for `count` more elements, growing geometrically as push_back() does so that repeated insertion stays // amortized. Returns false, with exceptions disabled, when they would exceed max_size() or the allocator declines; // the array is then unchanged. From 6faa360eb30168fd0de89b86d41296c326fa8428 Mon Sep 17 00:00:00 2001 From: Scott Dixon Date: Mon, 19 Oct 2026 17:22:16 -0700 Subject: [PATCH 4/4] Construct from an aliased VLA element in new storage, not a byte copy On the reallocate path, an argument that is an element of the array was byte-copied into an inactive union member and the element constructed from that copy. Before C++20, copying bytes does not start an object's lifetime, so reading the copy was undefined behaviour in the C++14 and C++17 configurations. Such an argument now takes the construct-in-new-storage-then-relocate path instead, so this rare case forgoes reallocate and no copy is needed. The element-argument overload also matched volatile arguments, because std::decay drops volatile, and then failed to compile when binding them to a const reference. It now matches only T and const T arguments; volatile ones take the general path. Tests now record every address the allocator constructs at during an append and check that &back() appears exactly once, because on the relocating path the new element is constructed before the old ones. They also expect aliased appends not to call reallocate, and cover a volatile argument. Addresses Codex review comments on #247. Co-Authored-By: Claude Opus 5.5 --- ...est_variable_length_array_reallocation.cpp | 44 +++++++++++++++---- include/cetl/variable_length_array.hpp | 33 +++++--------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp index 761c9e58..48cd6af3 100644 --- a/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp +++ b/cetlvast/suites/unittest/test_variable_length_array_reallocation.cpp @@ -8,6 +8,7 @@ #include "cetl/pf17/memory_resource.hpp" #include "cetlvast/helpers_gtest.hpp" +#include #include #include #include @@ -717,7 +718,8 @@ void expect_appended_elements_survive_growth(const bool decline) expected.push_back(expected.front()); } EXPECT_NE(array.data(), data_before); - EXPECT_EQ(resource.reallocations, reallocations + (std::is_trivially_copyable::value ? 1U : 0U)); + // An argument that is an element is constructed from in new storage, never through reallocate. + EXPECT_EQ(resource.reallocations, reallocations); } } ASSERT_EQ(array.size(), expected.size()); @@ -782,9 +784,9 @@ TEST(VLAReallocationGrowth, FailedAllocationOnAppendPreservesStorage) struct HookCounts { - std::size_t constructs = 0; - std::size_t destroys = 0; - const void* last_constructed = nullptr; + std::size_t constructs = 0; + std::size_t destroys = 0; + std::vector addresses; }; // A reallocate-capable allocator that counts its construct and destroy hooks. @@ -811,7 +813,7 @@ class HookCountingAllocator : public cetl::pf17::pmr::polymorphic_allocator void construct(U* pointer, Args&&... args) { ++counts_->constructs; - counts_->last_constructed = pointer; + counts_->addresses.push_back(pointer); Base::construct(pointer, std::forward(args)...); } @@ -853,6 +855,7 @@ TEST(VLAReallocationGrowth, AppendConstructsOnceInPlaceThroughAllocator) ASSERT_EQ(counts.constructs, AppendInitialCapacity); const int value = aliased ? array.front() : element_value(AppendInitialCapacity); const auto reallocations = resource.reallocations; + counts.addresses.clear(); if (aliased) { array.push_back(static_cast(array.front())); @@ -861,10 +864,13 @@ TEST(VLAReallocationGrowth, AppendConstructsOnceInPlaceThroughAllocator) { array.push_back(value); } - EXPECT_EQ(resource.reallocations, reallocations + 1U); - // When reallocate is declined, the fallback also relocates each old element through the allocator. - EXPECT_EQ(counts.constructs, AppendInitialCapacity + 1U + (decline ? AppendInitialCapacity : 0U)); - EXPECT_EQ(counts.last_constructed, &array.back()); + // An aliased argument is constructed from in new storage, never through reallocate. Without reallocate, + // each old element is also relocated through the allocator. + const bool relocates = aliased || decline; + EXPECT_EQ(resource.reallocations, reallocations + (aliased ? 0U : 1U)); + EXPECT_EQ(counts.constructs, AppendInitialCapacity + 1U + (relocates ? AppendInitialCapacity : 0U)); + const void* const appended = &array.back(); + EXPECT_EQ(std::count(counts.addresses.begin(), counts.addresses.end(), appended), 1); EXPECT_EQ(counts.destroys, 0U); EXPECT_EQ(array.back(), value); @@ -902,6 +908,26 @@ TEST(VLAReallocationGrowth, AppendConstructsOnceInPlaceThroughAllocator) } } +// std::decay drops volatile, but a volatile argument cannot be checked for aliasing through a const reference. It +// takes the general path, which constructs from it in new storage. +TEST(VLAReallocationGrowth, VolatileArgumentAppends) +{ + MovingResource resource; + Array array{typename Array::allocator_type{&resource}}; + array.reserve(AppendInitialCapacity); + for (std::size_t i = 0; i < AppendInitialCapacity; ++i) + { + array.push_back(element_value(i)); + } + volatile int value = FillValue; + array.emplace_back(value); + ASSERT_EQ(array.size(), AppendInitialCapacity + 1U); + for (std::size_t i = 0; i < array.size(); ++i) + { + EXPECT_EQ(array[i], i < AppendInitialCapacity ? element_value(i) : FillValue); + } +} + // Trivially copyable, but its converting constructor records where it ran, as a constructor that stores `this` does. struct Located { diff --git a/include/cetl/variable_length_array.hpp b/include/cetl/variable_length_array.hpp index 30479eec..d224bb80 100644 --- a/include/cetl/variable_length_array.hpp +++ b/include/cetl/variable_length_array.hpp @@ -1119,38 +1119,25 @@ class VariableLengthArrayBase return true; } + /// Whether `Arg&&` binds to a value_type that is not volatile, and so can be checked for aliasing. + template + using is_element_argument = std::is_same>, value_type>; + /// A single element-typed argument, as from push_back(), can be checked for aliasing. Either way the element is - /// constructed once, through the allocator, in its final storage. - template , value_type>::value, int> = 0> + /// constructed once, through the allocator, in its final storage. Volatile arguments take the general path. + template ::value, int> = 0> bool grow_and_emplace_back_impl(std::true_type, const size_type max_size, Arg&& arg) { - if (!refers_to_element(arg)) + if (refers_to_element(arg)) { - if (!grow(max_size)) - { - return false; - } - std::allocator_traits::construct(alloc_, data_ + size_, std::forward(arg)); - ++size_; - return true; + // Reallocation may release the element, so construct from it in new storage before relocating. + return grow_and_emplace_back_impl(std::false_type{}, max_size, std::forward(arg)); } - // The argument is an element that reallocation may release. Copying the bytes of a trivially copyable object - // copies its value without running a constructor, so the copy stands in for the argument. - union Snapshot - { - Snapshot() noexcept {} - ~Snapshot() {} - value_type value; - } snapshot; - (void) std::memcpy(static_cast(std::addressof(snapshot.value)), - static_cast(std::addressof(arg)), - sizeof(value_type)); if (!grow(max_size)) { return false; } - // Pass the copy with the argument's value category so that the same constructor is selected. - std::allocator_traits::construct(alloc_, data_ + size_, static_cast(snapshot.value)); + std::allocator_traits::construct(alloc_, data_ + size_, std::forward(arg)); ++size_; return true; }