[libcxx-commits] [libcxx] [libc++][safety] Enhance exception safety for vector assignments in reallocation scenarios (PR #117516)
Peng Liu via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Nov 24 20:47:03 PST 2024
https://github.com/winner245 created https://github.com/llvm/llvm-project/pull/117516
### Overview
Current implementations of vector assignment operations—including the `copy`, `move`, `initializer_list`-assignment operators, and all overloads of the `assign` functions, as well as the C++23 `assign_range` function—by libc++ and MSVC-STL can lead to scenarios where the vector is emptied during reallocations triggered by assignments. While this behavior conforms to the standard, it can result in data loss and may not meet user expectations, as demonstrated in this [demo](https://godbolt.org/z/jh5YqMrzM).
The potential data loss due to the assignment operations arises because these assignments ultimately execute the following code snippet in the case of reallocations:
```cpp
__vdeallocate();
__vallocate(__recommend(__new_size));
__construct_at_end(__first, __last, __new_size);
```
This sequence is unsafe because destructing and deallocating the old vector via `__vdeallocate()` occur strictly before reallocating (`__vallocate`) and reconstructing (`__construct_at_end`) the new vector. However, both of the latter operations may throw exceptions, leaving the original vector emptied and hence resulting in data loss.
### Proposed changes and tests
This draft aims to enhance the exception safety of assignment operations during reallocations in `std::vector`. The proposed solution involves reordering the deletion of old vector to occur strictly after the successful construction of the new vector. This adjustment does not introduce any additional operations; rather, it reorders existing ones to provide strong exception guarantees for assignment operations (except move-assignment operator) during reallocations.
It is important to note that in the general case of no reallocations, these assignment operations cannot provide strong guarantees unless additional time and space can be afforded. Additionally, peak memory usage will increase during reallocations, similar to the behavior observed with `push_back`, `emplace_back`, `resize`, and `reserve`.
A comprehensive set of exceptions—including allocation exceptions, element construction exceptions, and iterator exceptions—has been tested. These tests verify the strong exception guarantee for vector assignments specifically in cases where reallocations are triggered by assignments.
### Feedback
To provide complete context for this work, I would like to reference my earlier [issue](https://github.com/microsoft/STL/issues/5106) and a related [PR](https://github.com/microsoft/STL/pull/5107) submitted to the MSVC project, which addressed the same enhancement for MSVC's implementations of the assignment operations. However, that PR did not garner interest from the MSVC community, as it was considered a non-goal for the MSVC project.
In this draft PR, I have implemented the same enhancement for the libc++ implementations. While I am uncertain whether this aligns with libc++'s goals, I believe it is worth proposing for consideration. Additionally, it is noteworthy that the libstdc++ implementation of `std::vector` adopts the same reordering of assignment operations to provide strong exception guarantees in the case of reallocations, justifying the validity of this approach.
In summary, this patch enhances the `copy`/`initializer_list`-assignment operators, the `assign` overloads, and C++23 `assign_range` of `std::vector` to provide strong exception guarantees during vector reallocations, without incurring extra overhead. I propose this for your consideration and welcome any discussions regarding this work. I invite feedback from the libc++ maintainers and contributors.
>From b22bc95260830c532c18ee9e1cd45ee3ce0f9f15 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Sun, 24 Nov 2024 23:08:46 -0500
Subject: [PATCH] Enhance safety for vector assignment operations
---
libcxx/include/__vector/vector.h | 18 +-
.../vector.cons/assign_exceptions.pass.cpp | 199 ++++++++++++++++++
libcxx/test/support/allocators.h | 44 ++++
libcxx/test/support/exception_test_helpers.h | 114 ++++++++++
libcxx/test/support/test_allocator.h | 50 +++++
5 files changed, 421 insertions(+), 4 deletions(-)
create mode 100644 libcxx/test/std/containers/sequences/vector/vector.cons/assign_exceptions.pass.cpp
create mode 100644 libcxx/test/support/exception_test_helpers.h
diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index ae3ea1de61de01..ed3697990f3871 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -1025,9 +1025,14 @@ vector<_Tp, _Allocator>::__assign_with_size(_ForwardIterator __first, _Sentinel
this->__destruct_at_end(__m);
}
} else {
+ __split_buffer<value_type, allocator_type&> __v(__recommend(__new_size), 0, __alloc_);
+ __v.__construct_at_end_with_size(__first, __new_size);
__vdeallocate();
- __vallocate(__recommend(__new_size));
- __construct_at_end(__first, __last, __new_size);
+ this->__begin_ = __v.__begin_;
+ this->__end_ = __v.__end_;
+ this->__cap_ = __v.__cap_;
+ __v.__first_ = __v.__begin_ = __v.__end_ = __v.__cap_ = nullptr;
+ __annotate_new(__new_size);
}
}
@@ -1041,9 +1046,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<_Tp, _Allocator>::assign(size_type __n
else
this->__destruct_at_end(this->__begin_ + __n);
} else {
+ __split_buffer<value_type, allocator_type&> __v(__recommend(__n), 0, __alloc_);
+ __v.__construct_at_end(__n, __u);
__vdeallocate();
- __vallocate(__recommend(static_cast<size_type>(__n)));
- __construct_at_end(__n, __u);
+ this->__begin_ = __v.__begin_;
+ this->__end_ = __v.__end_;
+ this->__cap_ = __v.__cap_;
+ __v.__first_ = __v.__begin_ = __v.__end_ = __v.__cap_ = nullptr;
+ __annotate_new(__n);
}
}
diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/assign_exceptions.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/assign_exceptions.pass.cpp
new file mode 100644
index 00000000000000..28755d2c76fa51
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/assign_exceptions.pass.cpp
@@ -0,0 +1,199 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <vector>
+
+// Check that vector assignments don't change the rhs vector when an operation throws an exception during rallocations triggered by assignments
+
+#include <cassert>
+#include <ranges>
+#include <vector>
+
+#include "allocators.h"
+#include "exception_test_helpers.h"
+#include "test_allocator.h"
+#include "test_macros.h"
+
+void test_allocation_exception() {
+ {
+ limited_alloc_wrapper<int> alloc1 = limited_allocator<int, 100>();
+ limited_alloc_wrapper<int> alloc2 = limited_allocator<int, 200>();
+ std::vector<int, limited_alloc_wrapper<int>> v(100, alloc1);
+ std::vector<int, limited_alloc_wrapper<int>> in(200, alloc2);
+ try { // Throw in copy-assignment operator during allocation
+ v = in;
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 100);
+ }
+
+ {
+ limited_alloc_wrapper<int> alloc1 = limited_allocator<int, 100>();
+ limited_alloc_wrapper<int> alloc2 = limited_allocator<int, 200>();
+ std::vector<int, limited_alloc_wrapper<int>> v(100, alloc1);
+ std::vector<int, limited_alloc_wrapper<int>> in(200, alloc2);
+ try { // Throw in move-assignment operator during allocation
+ v = std::move(in);
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 100);
+ }
+
+ {
+ std::vector<int, limited_allocator<int, 5>> v(5);
+ std::initializer_list<int> in{1, 2, 3, 4, 5, 6};
+ try { // Throw in operator=(initializer_list<value_type>) during allocation
+ v = in;
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 5);
+ }
+
+ {
+ std::vector<int, limited_allocator<int, 100>> v(100);
+ std::vector<int> in(101, 1);
+ try { // Throw in assign(_ForwardIterator, _ForwardIterator) during allocation
+ v.assign(in.begin(), in.end());
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 100);
+ }
+
+ {
+ std::vector<int, limited_allocator<int, 100>> v(100);
+ try { // Throw in assign(size_type, const_reference) during allocation
+ v.assign(101, 1);
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 100);
+ }
+
+ {
+ std::vector<int, limited_allocator<int, 5>> v(5);
+ std::initializer_list<int> in{1, 2, 3, 4, 5, 6};
+ try { // Throw in assign(initializer_list<value_type>) during allocation
+ v.assign(in);
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 5);
+ }
+
+#if TEST_STD_VER >= 23
+ {
+ std::vector<int, limited_allocator<int, 100>> v(100);
+ std::vector<int> in(101, 1);
+ try { // Throw in assign(_ForwardIterator, _ForwardIterator) during allocation
+ v.assign_range(in);
+ } catch (const std::exception&) {
+ }
+ assert(v.size() == 100);
+ }
+#endif
+}
+
+void test_construction_exception() {
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ std::vector<throwing_t> in(6, t);
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in copy-assignment operator from element type during construction
+ v = in;
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ NONPOCMAAllocator<throwing_t> alloc1(1);
+ NONPOCMAAllocator<throwing_t> alloc2(2);
+ std::vector<throwing_t, NONPOCMAAllocator<throwing_t>> in(6, t, alloc1);
+ std::vector<throwing_t, NONPOCMAAllocator<throwing_t>> v(3, t, alloc2);
+ try { // Throw in move-assignment operator from element type during construction
+ v = std::move(in);
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ std::initializer_list<throwing_t> in{t, t, t, t, t, t};
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in operator=(initializer_list<value_type>) from element type during construction
+ v = in;
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+
+ {
+ std::vector<int> v(3);
+ try { // Throw in assign(_ForwardIterator, _ForwardIterator) from forward iterator during construction
+ v.assign(throwing_iterator<int, std::forward_iterator_tag>(),
+ throwing_iterator<int, std::forward_iterator_tag>(6));
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ std::vector<throwing_t> in(6, t);
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in assign(_ForwardIterator, _ForwardIterator) from element type during construction
+ v.assign(in.begin(), in.end());
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+#if TEST_STD_VER >= 23
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ std::vector<throwing_t> in(6, t);
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in assign_range(_Range&&) from element type during construction
+ v.assign_range(in);
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+#endif
+ {
+ int throw_after = 4;
+ throwing_t t = throw_after;
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in assign(size_type, const_reference) from element type during construction
+ v.assign(6, t);
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+
+ {
+ int throw_after = 10;
+ throwing_t t = throw_after;
+ std::initializer_list<throwing_t> in{t, t, t, t, t, t};
+ std::vector<throwing_t> v(3, t);
+ try { // Throw in assign(initializer_list<value_type>) from element type during construction
+ v.assign(in);
+ } catch (int) {
+ }
+ assert(v.size() == 3);
+ }
+}
+
+int main() {
+ test_allocation_exception();
+ test_construction_exception();
+}
diff --git a/libcxx/test/support/allocators.h b/libcxx/test/support/allocators.h
index 02436fd9c35ef1..3d9d292abe36a6 100644
--- a/libcxx/test/support/allocators.h
+++ b/libcxx/test/support/allocators.h
@@ -251,6 +251,50 @@ using POCCAAllocator = MaybePOCCAAllocator<T, /*POCCAValue = */true>;
template <class T>
using NonPOCCAAllocator = MaybePOCCAAllocator<T, /*POCCAValue = */false>;
+template <class T, bool POCMAValue>
+class MaybePOCMAAllocator {
+ template <class, bool>
+ friend class MaybePOCMAAllocator;
+
+public:
+ using propagate_on_container_move_assignment = std::integral_constant<bool, POCMAValue>;
+ using value_type = T;
+
+ template <class U>
+ struct rebind {
+ using other = MaybePOCMAAllocator<U, POCMAValue>;
+ };
+
+ constexpr MaybePOCMAAllocator(int id) : id_(id) {}
+
+ template <class U>
+ constexpr MaybePOCMAAllocator(const MaybePOCMAAllocator<U, POCMAValue>& other) : id_(other.id_) {}
+
+ constexpr T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
+
+ constexpr void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p, n); }
+
+ constexpr int id() const { return id_; }
+
+ template <class U>
+ constexpr friend bool operator==(const MaybePOCMAAllocator& lhs, const MaybePOCMAAllocator<U, POCMAValue>& rhs) {
+ return lhs.id() == rhs.id();
+ }
+
+ template <class U>
+ constexpr friend bool operator!=(const MaybePOCMAAllocator& lhs, const MaybePOCMAAllocator<U, POCMAValue>& rhs) {
+ return !(lhs == rhs);
+ }
+
+private:
+ int id_;
+};
+
+template <class T>
+using POCMAAllocator = MaybePOCMAAllocator<T, /*POCMAValue = */ true>;
+template <class T>
+using NONPOCMAAllocator = MaybePOCMAAllocator<T, /*POCMAValue = */ false>;
+
#endif // TEST_STD_VER >= 11
#endif // ALLOCATORS_H
diff --git a/libcxx/test/support/exception_test_helpers.h b/libcxx/test/support/exception_test_helpers.h
new file mode 100644
index 00000000000000..54fde657bde0dd
--- /dev/null
+++ b/libcxx/test/support/exception_test_helpers.h
@@ -0,0 +1,114 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef EXCEPTION_TEST_HELPER_H
+#define EXCEPTION_TEST_HELPER_H
+
+#include "count_new.h"
+
+struct throwing_t {
+ int* throw_after_n_ = nullptr;
+ throwing_t() { throw 0; }
+
+ throwing_t(int& throw_after_n) : throw_after_n_(&throw_after_n) {
+ if (throw_after_n == 0)
+ throw 0;
+ --throw_after_n;
+ }
+
+ throwing_t(const throwing_t& rhs) : throw_after_n_(rhs.throw_after_n_) {
+ if (throw_after_n_ == nullptr || *throw_after_n_ == 0)
+ throw 1;
+ --*throw_after_n_;
+ }
+
+ throwing_t& operator=(const throwing_t& rhs) {
+ throw_after_n_ = rhs.throw_after_n_;
+ if (throw_after_n_ == nullptr || *throw_after_n_ == 0)
+ throw 1;
+ --*throw_after_n_;
+ return *this;
+ }
+
+ friend bool operator==(const throwing_t& lhs, const throwing_t& rhs) {
+ return lhs.throw_after_n_ == rhs.throw_after_n_;
+ }
+ friend bool operator!=(const throwing_t& lhs, const throwing_t& rhs) {
+ return lhs.throw_after_n_ != rhs.throw_after_n_;
+ }
+};
+
+template <class T>
+struct throwing_allocator {
+ using value_type = T;
+ using is_always_equal = std::false_type;
+
+ bool throw_on_copy_ = false;
+
+ throwing_allocator(bool throw_on_ctor = true, bool throw_on_copy = false) : throw_on_copy_(throw_on_copy) {
+ if (throw_on_ctor)
+ throw 0;
+ }
+
+ template <class U>
+ throwing_allocator(const throwing_allocator<U>& rhs) : throw_on_copy_(rhs.throw_on_copy_) {
+ if (throw_on_copy_)
+ throw 0;
+ }
+
+ T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
+ void deallocate(T* ptr, std::size_t n) { std::allocator<T>().deallocate(ptr, n); }
+
+ template <class U>
+ friend bool operator==(const throwing_allocator&, const throwing_allocator<U>&) {
+ return true;
+ }
+};
+
+template <class T, class IterCat>
+struct throwing_iterator {
+ using iterator_category = IterCat;
+ using difference_type = std::ptrdiff_t;
+ using value_type = T;
+ using reference = T&;
+ using pointer = T*;
+
+ int i_;
+ T v_;
+
+ throwing_iterator(int i = 0, const T& v = T()) : i_(i), v_(v) {}
+
+ reference operator*() {
+ if (i_ == 1)
+ throw 1;
+ return v_;
+ }
+
+ friend bool operator==(const throwing_iterator& lhs, const throwing_iterator& rhs) { return lhs.i_ == rhs.i_; }
+ friend bool operator!=(const throwing_iterator& lhs, const throwing_iterator& rhs) { return lhs.i_ != rhs.i_; }
+
+ throwing_iterator& operator++() {
+ ++i_;
+ return *this;
+ }
+
+ throwing_iterator operator++(int) {
+ auto tmp = *this;
+ ++i_;
+ return tmp;
+ }
+};
+
+inline void check_new_delete_called() {
+ assert(globalMemCounter.new_called == globalMemCounter.delete_called);
+ assert(globalMemCounter.new_array_called == globalMemCounter.delete_array_called);
+ assert(globalMemCounter.aligned_new_called == globalMemCounter.aligned_delete_called);
+ assert(globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
+}
+
+#endif // EXCEPTION_TEST_HELPER_H
\ No newline at end of file
diff --git a/libcxx/test/support/test_allocator.h b/libcxx/test/support/test_allocator.h
index dcd15332ca304f..2e2aca54938155 100644
--- a/libcxx/test/support/test_allocator.h
+++ b/libcxx/test/support/test_allocator.h
@@ -480,6 +480,56 @@ TEST_CONSTEXPR inline bool operator!=(limited_allocator<T, N> const& LHS, limite
return !(LHS == RHS);
}
+// type erasure wrapper for limited_allocator<T, N>
+template <typename T>
+class limited_alloc_wrapper {
+ template <class S, class U>
+ friend bool operator==(const limited_alloc_wrapper<S>&, const limited_alloc_wrapper<U>&);
+
+public:
+ typedef T value_type;
+ typedef value_type* pointer;
+ typedef const value_type* const_pointer;
+ typedef value_type& reference;
+ typedef const value_type& const_reference;
+ typedef std::size_t size_type;
+ typedef std::ptrdiff_t difference_type;
+
+ template <typename Alloc>
+ limited_alloc_wrapper(Alloc&& a) : pimpl{std::make_shared<impl_type<Alloc>>(std::forward<Alloc>(a))} {}
+
+ limited_alloc_wrapper(const limited_alloc_wrapper& other) = default;
+
+ pointer allocate(std::size_t n) { return pimpl->allocate(n); }
+ void deallocate(pointer p, size_t n) { pimpl->deallocate(p, n); }
+ size_type max_size() const { return pimpl->max_size(); }
+
+private:
+ struct impl_base {
+ virtual pointer allocate(std::size_t n) = 0;
+ virtual void deallocate(pointer p, size_t n) = 0;
+ virtual size_type max_size() const = 0;
+ virtual ~impl_base() {}
+ };
+
+ std::shared_ptr<impl_base> pimpl;
+
+ template <typename Alloc>
+ struct impl_type : impl_base {
+ Alloc a;
+ impl_type(const Alloc& a_) : a{a_} {}
+
+ pointer allocate(std::size_t n) override { return a.allocate(n); }
+ void deallocate(pointer p, size_t n) override { a.deallocate(p, n); }
+ size_type max_size() const override { return a.max_size(); }
+ };
+};
+
+template <class T, class U>
+bool operator==(const limited_alloc_wrapper<T>& lhs, const limited_alloc_wrapper<U>& rhs) {
+ return lhs.pimpl == rhs.pimpl;
+}
+
// Track the "provenance" of this allocator instance: how many times was
// select_on_container_copy_construction called in order to produce it?
//
More information about the libcxx-commits
mailing list