[libc++] Destroy elements when exceptions are thrown in __construct_at_end

[kisuhorikka]

Changes:

• Destruct new created elements in _ConstructTransaction if exceptions raised.

Fix #59293

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: None (kisuhorikka)

Changes

Changes:

• Destruct new created elements in _ConstructTransaction if exceptions raised.

Fix #59293

---

Full diff: https://github.com/llvm/llvm-project/pull/167112.diff

2 Files Affected:

• (modified) libcxx/include/__vector/vector.h (+3-1)
• (added) libcxx/test/std/containers/sequences/vector/exception_construct_at_end.pass.cpp (+335)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 316d3a9d10eff..a1636c9be5ebc 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -749,19 +749,21 @@ class vector {
 
   struct _ConstructTransaction {
     _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI explicit _ConstructTransaction(vector& __v, size_type __n)
-        : __v_(__v), __pos_(__v.__end_), __new_end_(__v.__end_ + __n) {
+        : __v_(__v), __pos_(__v.__end_), __old_end_(__v.__end_), __new_end_(__v.__end_ + __n) {
       __v_.__annotate_increase(__n);
     }
 
     _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI ~_ConstructTransaction() {
       __v_.__end_ = __pos_;
       if (__pos_ != __new_end_) {
+        __v_.__destruct_at_end(__old_end_);
         __v_.__annotate_shrink(__new_end_ - __v_.__begin_);
       }
     }
 
     vector& __v_;
     pointer __pos_;
+    pointer const __old_end_;
     const_pointer const __new_end_;
 
     _ConstructTransaction(_ConstructTransaction const&)            = delete;
diff --git a/libcxx/test/std/containers/sequences/vector/exception_construct_at_end.pass.cpp b/libcxx/test/std/containers/sequences/vector/exception_construct_at_end.pass.cpp
new file mode 100644
index 0000000000000..af648f397b0f2
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/vector/exception_construct_at_end.pass.cpp
@@ -0,0 +1,335 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+// <vector>
+
+// Make sure elements are destroyed when exceptions thrown in __construct_at_end
+
+#include <cassert>
+#include <cstddef>
+#include <memory>
+#include <vector>
+
+#include "test_macros.h"
+#if TEST_STD_VER >= 20
+#  include <ranges>
+#endif
+
+#include "common.h"
+#include "count_new.h"
+
+struct throw_context {
+  static int num;
+  static int limit;
+
+  throw_context(int lim = 2) {
+    num   = 0;
+    limit = lim;
+  }
+
+  static void inc() {
+    ++num;
+    if (num >= limit) {
+      --num;
+      throw 1;
+    }
+  }
+
+  static void dec() { --num; }
+};
+
+int throw_context::num   = 0;
+int throw_context::limit = 0;
+
+int debug = 0;
+
+class throw_element {
+public:
+  throw_element() : data(new int(1)) {
+    try {
+      throw_context::inc();
+    } catch (int) {
+      delete data;
+      throw;
+    }
+  }
+
+  throw_element([[maybe_unused]] throw_element const& other) : data(new int(1)) {
+    try {
+      throw_context::inc();
+    } catch (int) {
+      delete data;
+      throw;
+    }
+  }
+
+  ~throw_element() {
+    if (data) {
+      delete data;
+      throw_context::dec();
+      if (debug)
+        printf("dctor\n");
+    }
+  }
+
+  throw_element& operator=([[maybe_unused]] throw_element const& other) {
+    // nothing to do
+    return *this;
+  }
+
+private:
+  int* data;
+};
+
+int main(int, char*[]) {
+  using AllocType = std::allocator<throw_element>;
+
+  // vector(size_type __n)
+  {
+    throw_context ctx;
+    try {
+      std::vector<throw_element> v(3);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(size_type __n, const allocator_type& __a)
+  {
+    throw_context ctx;
+    AllocType alloc;
+    try {
+      std::vector<throw_element> v(3, alloc);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(size_type __n, const value_type& __x)
+  {
+    throw_context ctx(3);
+    try {
+      throw_element e;
+      std::vector<throw_element> v(3, e);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(size_type __n, const value_type& __x, const allocator_type& __a)
+  {
+    throw_context ctx(3);
+    try {
+      throw_element e;
+      AllocType alloc;
+      std::vector<throw_element> v(4, e, alloc);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(_ForwardIterator __first, _ForwardIterator __last)
+  {
+    throw_context ctx(4);
+    try {
+      std::vector<throw_element> v1(2);
+      std::vector<throw_element> v2(v1.begin(), v1.end());
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a)
+  {
+    throw_context ctx(4);
+    AllocType alloc;
+    try {
+      std::vector<throw_element> v1(2);
+      std::vector<throw_element> v2(v1.begin(), v1.end(), alloc);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+#if _LIBCPP_STD_VER >= 23
+  // vector(from_range_t, _Range&& __range, const allocator_type& __alloc = allocator_type())
+  {
+    throw_context ctx(4);
+    try {
+      std::vector<throw_element> r(2);
+      std::vector<throw_element> v(std::from_range, std::views::counted(r.begin(), 2));
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+#endif
+
+  // vector(const vector& __x)
+  {
+    throw_context ctx(4);
+    try {
+      std::vector<throw_element> v1(2);
+      std::vector<throw_element> v2(v1);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(initializer_list<value_type> __il)
+  {
+    throw_context ctx(6);
+    try {
+      throw_element e;
+      std::vector<throw_element> v({e, e, e});
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // vector(initializer_list<value_type> __il, const allocator_type& __a)
+  {
+    throw_context ctx(6);
+    AllocType alloc;
+    try {
+      throw_element e;
+      std::vector<throw_element> v({e, e, e}, alloc);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+
+  // void resize(size_type __sz)
+  {
+    // cap < size
+    throw_context ctx;
+    std::vector<throw_element> v;
+    v.reserve(1);
+    try {
+      v.resize(4);
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+  // void resize(size_type __sz, const_reference __x)
+  {
+    // cap < size
+    throw_context ctx(3);
+    std::vector<throw_element> v;
+    v.reserve(1);
+    try {
+      throw_element e;
+      v.resize(4, e);
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+  // void assign(_ForwardIterator __first, _ForwardIterator __last)
+  {
+    // new size <= cap && new size > size
+    throw_context ctx(4);
+    std::vector<throw_element> v;
+    v.reserve(3);
+    try {
+      std::vector<throw_element> data(2);
+      v.assign(data.begin(), data.end());
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+  {
+    // new size > cap
+    throw_context ctx(4);
+    std::vector<throw_element> v;
+    try {
+      std::vector<throw_element> data(2);
+      v.assign(data.begin(), data.end());
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+#if _LIBCPP_STD_VER >= 23
+  // void assign_range(_Range&& __range)
+  {
+    throw_context ctx(5);
+    std::vector<throw_element> v;
+    try {
+      std::vector<throw_element> r(3);
+      v.assign_range(r);
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+#endif
+
+  // vector& operator=(initializer_list<value_type> __il)
+  {
+    throw_context ctx(5);
+    std::vector<throw_element> v;
+    try {
+      throw_element e;
+      v = {e, e};
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+  // vector<_Tp, _Allocator>& vector<_Tp, _Allocator>::operator=(const vector& __x)
+  {
+    throw_context ctx(4);
+    std::vector<throw_element> v;
+    try {
+      std::vector<throw_element> data(2);
+      v = data;
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+  // iterator insert(const_iterator __position, _ForwardIterator __first, _ForwardIterator __last)
+  {
+    throw_context ctx(6);
+    std::vector<throw_element> v;
+    v.reserve(10);
+    try {
+      std::vector<throw_element> data(3);
+      v.insert(v.begin(), data.begin(), data.end());
+    } catch (int) {
+    }
+    assert(globalMemCounter.new_called == globalMemCounter.delete_called + 1);
+  }
+  check_new_delete_called();
+
+#if _LIBCPP_STD_VER >= 23
+  // iterator insert_range(const_iterator __position, _Range&& __range)
+  {
+    throw_context ctx(3);
+    std::vector<throw_element> v;
+    try {
+      std::vector<throw_element> data(2);
+      v.insert_range(v.begin(), data);
+    } catch (int) {
+    }
+    check_new_delete_called();
+  }
+#endif
+
+  return 0;
+}
\ No newline at end of file

[philnik777]

I think we should instead use the appropriate __uninitialized_allocator functions inside __construct_at_end. That would allow us to get rid of _ConstructTransaction at some point.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[frederick-vs-ja]

Let's use the conventional names. (Also, it seems possible to move this into <__vector/vector.h>, but perhaps it's also OK to keep it here for consistency,)

Suggested change

	__uninitialized_fill_n_with_alloc(_Alloc& __alloc, _ForwardIterator __first, _Size __n, const _Tp& __x) {
	__uninitialized_allocator_fill_n(_Alloc& __alloc, _ForwardIterator __first, _Size __n, const _Tp& __x) {

[frederick-vs-ja]

Suggested change

	// __uninitialized_default_construct_n_with_alloc
	template <class _Alloc, class _ForwardIterator, class _Size>
	inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _ForwardIterator
	__uninitialized_default_construct_n_with_alloc(_Alloc& __alloc, _ForwardIterator __first, _Size __n) {
	// __uninitialized_allocator_value_construct_n
	template <class _Alloc, class _ForwardIterator, class _Size>
	inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _ForwardIterator
	__uninitialized_allocator_value_construct_n(_Alloc& __alloc, _ForwardIterator __first, _Size __n) {

[frederick-vs-ja]

Template parameters should be deduced.

Suggested change

	__tx.__pos_ = std::__uninitialized_default_construct_n_with_alloc<_Allocator>(this->__alloc_, this->__end_, __n);
	__tx.__pos_ = std::__uninitialized_allocator_value_construct_n(this->__alloc_, this->__end_, __n);

[frederick-vs-ja]

Suggested change

	__tx.__pos_ = std::__uninitialized_fill_n_with_alloc<_Allocator>(this->__alloc_, this->__end_, __n, __x);
	__tx.__pos_ = std::__uninitialized_allocator_fill_n(this->__alloc_, this->__end_, __n, __x);