[libc++][test] Fix and refactor exception tests for std::vector constructors

[winner245]

The existing exceptions tests for vector<T> have several issues: some tests did not throw exceptions at all, making them not useful for exception-safety testing, and some tests did not throw exceptions at the intended points, failing to serve their expected purpose. This PR fixes those tests for vector's constructors. Morever, this PR extracted common classes and utilities into a separate header file, and renamed those classes using more descriptive names.

For your convenience, I have commented on the problematic tests in what follows.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The existing exceptions tests for vector<T> have several issues: some tests did not throw exceptions at all, making them not useless for exception-safety testing, and some tests did not throw exceptions at the intended points, failing to serve their expected purpose. This PR fixes all those tests. Morever, this PR extracted common classes and utilities into a separate header file, and renamed those classes using more descriptive names.

For your convenience, I have added comments on the problematic tests.

---

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

2 Files Affected:

• (modified) libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp (+35-111)
• (added) libcxx/test/support/exception_test_helpers.h (+119)

diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp
index e2b0d691889c6c..82768947986a27 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp
@@ -17,94 +17,12 @@
 #include <vector>
 
 #include "count_new.h"
+#include "exception_test_helpers.h"
+#include "test_allocator.h"
 #include "test_iterators.h"
 
-template <class T>
-struct Allocator {
-  using value_type      = T;
-  using is_always_equal = std::false_type;
-
-  template <class U>
-  Allocator(const Allocator<U>&) {}
-
-  Allocator(bool should_throw = true) {
-    if (should_throw)
-      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 Allocator&, const Allocator<U>&) { return true; }
-};
-
-struct ThrowingT {
-  int* throw_after_n_ = nullptr;
-  ThrowingT() { throw 0; }
-
-  ThrowingT(int& throw_after_n) : throw_after_n_(&throw_after_n) {
-    if (throw_after_n == 0)
-      throw 0;
-    --throw_after_n;
-  }
-
-  ThrowingT(const ThrowingT& rhs) : throw_after_n_(rhs.throw_after_n_) {
-    if (throw_after_n_ == nullptr || *throw_after_n_ == 0)
-      throw 1;
-    --*throw_after_n_;
-  }
-
-  ThrowingT& operator=(const ThrowingT& rhs) {
-    throw_after_n_ = rhs.throw_after_n_;
-    if (throw_after_n_ == nullptr || *throw_after_n_ == 0)
-      throw 1;
-    --*throw_after_n_;
-    return *this;
-  }
-};
-
-template <class IterCat>
-struct Iterator {
-  using iterator_category = IterCat;
-  using difference_type   = std::ptrdiff_t;
-  using value_type        = int;
-  using reference         = int&;
-  using pointer           = int*;
-
-  int i_;
-  Iterator(int i = 0) : i_(i) {}
-  int& operator*() {
-    if (i_ == 1)
-      throw 1;
-    return i_;
-  }
-
-  friend bool operator==(const Iterator& lhs, const Iterator& rhs) { return lhs.i_ == rhs.i_; }
-
-  friend bool operator!=(const Iterator& lhs, const Iterator& rhs) { return lhs.i_ != rhs.i_; }
-
-  Iterator& operator++() {
-    ++i_;
-    return *this;
-  }
-
-  Iterator operator++(int) {
-    auto tmp = *this;
-    ++i_;
-    return tmp;
-  }
-};
-
-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);
-}
-
 int main(int, char**) {
-  using AllocVec = std::vector<int, Allocator<int> >;
+  using AllocVec = std::vector<int, throwing_allocator<int> >;
   try { // vector()
     AllocVec vec;
   } catch (int) {
@@ -112,7 +30,7 @@ int main(int, char**) {
   check_new_delete_called();
 
   try { // Throw in vector(size_type) from type
-    std::vector<ThrowingT> get_alloc(1);
+    std::vector<throwing_t> get_alloc(1);
   } catch (int) {
   }
   check_new_delete_called();
@@ -120,42 +38,44 @@ int main(int, char**) {
 #if TEST_STD_VER >= 14
   try { // Throw in vector(size_type, value_type) from type
     int throw_after = 1;
-    ThrowingT v(throw_after);
-    std::vector<ThrowingT> get_alloc(1, v);
+    throwing_t v(throw_after);
+    std::vector<throwing_t> get_alloc(1, v);
   } catch (int) {
   }
   check_new_delete_called();
 
-  try { // Throw in vector(size_type, const allocator_type&) from allocator
-    Allocator<int> alloc(false);
+  try {                                         // Throw in vector(size_type, const allocator_type&) from allocator
+    throwing_allocator<int> alloc(false, true); // throw on copy only
     AllocVec get_alloc(0, alloc);
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(size_type, const allocator_type&) from the type
-    std::vector<ThrowingT> vec(1, std::allocator<ThrowingT>());
+    std::vector<throwing_t> vec(1, std::allocator<throwing_t>());
   } catch (int) {
   }
   check_new_delete_called();
-#endif  // TEST_STD_VER >= 14
+#endif // TEST_STD_VER >= 14
 
   try { // Throw in vector(size_type, value_type, const allocator_type&) from the type
     int throw_after = 1;
-    ThrowingT v(throw_after);
-    std::vector<ThrowingT> vec(1, v, std::allocator<ThrowingT>());
+    throwing_t v(throw_after);
+    std::vector<throwing_t> vec(1, v, std::allocator<throwing_t>());
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(InputIterator, InputIterator) from input iterator
-    std::vector<int> vec((Iterator<std::input_iterator_tag>()), Iterator<std::input_iterator_tag>(2));
+    std::vector<int> vec(
+        (throwing_iterator<int, std::input_iterator_tag>()), throwing_iterator<int, std::input_iterator_tag>(2));
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(InputIterator, InputIterator) from forward iterator
-    std::vector<int> vec((Iterator<std::forward_iterator_tag>()), Iterator<std::forward_iterator_tag>(2));
+    std::vector<int> vec(
+        (throwing_iterator<int, std::forward_iterator_tag>()), throwing_iterator<int, std::forward_iterator_tag>(2));
   } catch (int) {
   }
   check_new_delete_called();
@@ -169,21 +89,24 @@ int main(int, char**) {
 
   try { // Throw in vector(InputIterator, InputIterator, const allocator_type&) from input iterator
     std::allocator<int> alloc;
-    std::vector<int> vec(Iterator<std::input_iterator_tag>(), Iterator<std::input_iterator_tag>(2), alloc);
+    std::vector<int> vec(
+        throwing_iterator<int, std::input_iterator_tag>(), throwing_iterator<int, std::input_iterator_tag>(2), alloc);
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(InputIterator, InputIterator, const allocator_type&) from forward iterator
     std::allocator<int> alloc;
-    std::vector<int> vec(Iterator<std::forward_iterator_tag>(), Iterator<std::forward_iterator_tag>(2), alloc);
+    std::vector<int> vec(throwing_iterator<int, std::forward_iterator_tag>(),
+                         throwing_iterator<int, std::forward_iterator_tag>(2),
+                         alloc);
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(InputIterator, InputIterator, const allocator_type&) from allocator
     int a[] = {1, 2};
-    Allocator<int> alloc(false);
+    throwing_allocator<int> alloc(false, true); // throw on copy only
     AllocVec vec(cpp17_input_iterator<int*>(a), cpp17_input_iterator<int*>(a + 2), alloc);
   } catch (int) {
     // FIXME: never called.
@@ -192,7 +115,7 @@ int main(int, char**) {
 
   try { // Throw in vector(InputIterator, InputIterator, const allocator_type&) from allocator
     int a[] = {1, 2};
-    Allocator<int> alloc(false);
+    throwing_allocator<int> alloc(false, true); // throw on copy only
     AllocVec vec(forward_iterator<int*>(a), forward_iterator<int*>(a + 2), alloc);
   } catch (int) {
     // FIXME: never called.
@@ -200,8 +123,8 @@ int main(int, char**) {
   check_new_delete_called();
 
   try { // Throw in vector(const vector&) from type
-    std::vector<ThrowingT> vec;
-    int throw_after = 0;
+    std::vector<throwing_t> vec;
+    int throw_after = 1;
     vec.emplace_back(throw_after);
     auto vec2 = vec;
   } catch (int) {
@@ -209,19 +132,20 @@ int main(int, char**) {
   check_new_delete_called();
 
   try { // Throw in vector(const vector&, const allocator_type&) from type
-    std::vector<ThrowingT> vec;
+    std::vector<throwing_t> vec;
     int throw_after = 1;
     vec.emplace_back(throw_after);
-    std::vector<ThrowingT> vec2(vec, std::allocator<int>());
+    std::vector<throwing_t> vec2(vec, std::allocator<int>());
   } catch (int) {
   }
   check_new_delete_called();
 
-  try { // Throw in vector(vector&&, const allocator_type&) from type
-    std::vector<ThrowingT, Allocator<ThrowingT> > vec(Allocator<ThrowingT>(false));
-    int throw_after = 1;
-    vec.emplace_back(throw_after);
-    std::vector<ThrowingT, Allocator<ThrowingT> > vec2(std::move(vec), Allocator<ThrowingT>(false));
+  try { // Throw in vector(vector&&, const allocator_type&) from type during element-wise move
+    std::vector<throwing_t, test_allocator<throwing_t> > vec(test_allocator<throwing_t>(1));
+    int throw_after = 10;
+    throwing_t v(throw_after);
+    vec.insert(vec.end(), 6, v);
+    std::vector<throwing_t, test_allocator<throwing_t> > vec2(std::move(vec), test_allocator<throwing_t>(2));
   } catch (int) {
   }
   check_new_delete_called();
@@ -229,14 +153,14 @@ int main(int, char**) {
 #if TEST_STD_VER >= 11
   try { // Throw in vector(initializer_list<value_type>) from type
     int throw_after = 1;
-    std::vector<ThrowingT> vec({ThrowingT(throw_after)});
+    std::vector<throwing_t> vec({throwing_t(throw_after)});
   } catch (int) {
   }
   check_new_delete_called();
 
   try { // Throw in vector(initializer_list<value_type>, const allocator_type&) constructor from type
     int throw_after = 1;
-    std::vector<ThrowingT> vec({ThrowingT(throw_after)}, std::allocator<ThrowingT>());
+    std::vector<throwing_t> vec({throwing_t(throw_after)}, std::allocator<throwing_t>());
   } catch (int) {
   }
   check_new_delete_called();
diff --git a/libcxx/test/support/exception_test_helpers.h b/libcxx/test/support/exception_test_helpers.h
new file mode 100644
index 00000000000000..b7c2e49a5f7dba
--- /dev/null
+++ b/libcxx/test/support/exception_test_helpers.h
@@ -0,0 +1,119 @@
+//===----------------------------------------------------------------------===//
+//
+// 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_HELPERS_H
+#define EXCEPTION_TEST_HELPERS_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;
+  }
+
+  throwing_allocator(const throwing_allocator& rhs) : throw_on_copy_(rhs.throw_on_copy_) {
+    if (throw_on_copy_)
+      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_HELPERS_H
\ No newline at end of file

[ldionne]

Thanks a lot for the fixes, these tests are extremely important so it's good to fix them. Do you have an understanding of whether our tests for other (non-construction) operations are sufficient in terms of exceptions-testing?

[ldionne]

Can we land the vector<bool> patch and then rebase this on top of it?

If you want to land this one first instead, please rebase it on top of main and let me know when you're ready for me to have a look. We can then land this one and then rebase the vector<bool> one on top of everything.

[winner245]

Sure, we can merge vector<bool> first, and then let this one rebase. But before that, let me first rebase vector<bool>.

[winner245]

Thanks a lot for the fixes, these tests are extremely important so it's good to fix them. Do you have an understanding of whether our tests for other (non-construction) operations are sufficient in terms of exceptions-testing?

Thank you for the question! I am glad that you asked, as it gives me an opportunity to elaborate on my recent work regarding exception safety. I have done a thorough analysis of vector, particularly focusing on exception safety. I found that we are lacking some critical exception tests, and some existing ones are flawed. I believe all vector operations providing strong exception guarantees should be tested under various exception scenarios. Here are my findings:

Operations that provide strong guarantees:

• Constructors: We have decent exception test coverage for constructors, though a few important ones are either missing or problematic. With my recent PRs, I believe we have covered all constructors pretty well for both vector and vector<bool>.
• resize, reserve, and shrink_to_fit: Currently, there is only one exception test for reserve in reserve.pass.cpp, which only considers the allocation exception. Other sources of exceptions, such as element type exceptions during the relocation process, are not covered. Additionally, no exception tests are provided for resize and shrink_to_fit, althoug they are subject to the same sources of exceptions.
• emplace_back and push_back: Our current implementation of push_back calls emplace_back. Therefore, at least one of these operations requires a comprehensive exception test coverage. However, there is only one test for push_back in push_back_exception_safety.pass.cpp, which only considers copy-ctor exceptions, neglecting other exceptions such as allocation exception or other direct ctor exceptions.

Remaining operations

What remains to analyze are the assignment functions, insertion functions, and erasure functions, which do not provide exception strong guarantees in general.

• Erasure functions: pop_back and clear do not throw exceptions, and thus do not need exception testing. The erase overloads throw only when the the assignment operators throw during the movement of the tail elements to the front after removal. So this boils down to the assignment operators.
• Assignment functions (including operator=, all assign overloads, and C++23's assign_range): The standard does not mandate strong exception guarantees, hence we do not have any exception tests coverage. However, my recent study indicated that we can achieve strong guarantees for assignment functions specifically during reallocations, without introducing any additional operations. I reviewed the implementations in GCC and MSVC and found that GCC has already adopted a similar approach to mine for providing strong guarantees in this context. This motivated my latest PR, which also includes a comprehensive coverage of all sources of exceptions for the assignment functions. I truly appreciate your positive feedback on this work and hope it generates interest in evaluating whether this improvement is necessary for libc++.
• Insertion functions (including all insert overloads and C++23's insert_range): The standard only requires these functions to provide a strong exception guarantee when the insertion is done at the end to insert a single-element (e.g., a.insert(a.end(), x)). Following my work on the assignment functions, I also developed a method to make all these insertion functions provide strong exception guarantees during reallocations. Again, this improvement is based on some reordering tricks but in a more tricky way, thus introducing no extra time cost. I will await the outcome of my PR for assignment functions before deciding whether to submit this one for review.

This summarizes my current understanding of exception safety for vector. I would love to hear if you have any comments or suggestions on my work.

[github-actions]

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

[ldionne]

Thanks a lot for the analysis! What I would find amazingly useful would be to first augment the tests for vector to ensure that all the exception safety behavior is tested properly, and we can then proceed to gradually refactor vector's implementation to either make it faster (for example via some relocation work like this WIP branch of mine) or to change the exception safety guarantee that we provide in libc++ (your PR), depending on what we decide to do. Augmenting the tests first has the benefit that we'll gain confidence that our follow-up changes are either not functionally-changing, or they are but exactly in the ways that we expect.

Would you like to do that?

[ldionne]

Thanks for the patch! I have some questions but otherwise this LGTM.

[winner245]

Thanks a lot for the analysis! What I would find amazingly useful would be to first augment the tests for vector to ensure that all the exception safety behavior is tested properly, and we can then proceed to gradually refactor vector's implementation to either make it faster (for example via some relocation work like this WIP branch of mine) or to change the exception safety guarantee that we provide in libc++ (your PR), depending on what we decide to do. Augmenting the tests first has the benefit that we'll gain confidence that our follow-up changes are either not functionally-changing, or they are but exactly in the ways that we expect.

Would you like to do that?

Thank you for your feedback! I completely agree that augmenting the tests for vector to ensure thorough exception safety coverage is a crucial first step. This will indeed provide us with the confidence needed for any subsequent changes, whether they aim to improve performance or enhance exception safety guarantees.

I would be happy to proceed with augmenting the tests as suggested. I look forward to collaborating on this!

[ldionne]

LGTM with the FIXME comment removed (and CI passing, as always). Thank you!