[libc++][vector] Make constructor vector(count, value, allocator) exception-safe

[mtezych]

The constructor std::vector(count, value, allocator) was missing an exception guard,
responsible for destroying successfully created elements, in case an exception was thrown:

• https://godbolt.org/z/MeK37n8fb

Since all constructors filling std::vector<> with elements of the same value:

• vector(count)
• vector(count, allocator)
• vector(count, value)
• vector(count, value, allocator)

have nearly identical implementation, their definitions should be physically close to each other.

In order to achieve that goal, the constructor std::vector(count, value, allocator) is now defined
outside of the std::vector<> class template and has sightly different enable_if<> expression,
matching to those used in the constructors taking two iterators:

• vector(iterator first, iterator last)
• vector(iterator first, iterator last, allocator)

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mateusz Zych (mtezych)

Changes

The constructor std::vector(count, value, allocator) was missing an exception guard,
responsible for destroying successfully created elements, in case an exception was thrown:

• https://godbolt.org/z/MeK37n8fb

Since all constructors filling std::vector<> with elements of the same value:

• vector(count)
• vector(count, allocator)
• vector(count, value)
• vector(count, value, allocator)

have nearly identical implementation, their definitions should be physically close to each other.

In order to achieve that goal, the constructor std::vector(count, value, allocator) is now defined
outside of the std::vector<> class template and has sightly different enable_if<> expression,
matching to those used in the constructors taking two iterators:

• vector(iterator first, iterator last)
• vector(iterator first, iterator last, allocator)

---

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

1 Files Affected:

• (modified) libcxx/include/vector (+15-8)

diff --git a/libcxx/include/vector b/libcxx/include/vector
index ce7df7a9f04207..62d36d9318bd69 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -429,15 +429,9 @@ public:
 #endif
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(size_type __n, const value_type& __x);
 
-  template <class = __enable_if_t<__is_allocator<_Allocator>::value> >
+  template <__enable_if_t<__is_allocator<_Allocator>::value, int> = 0>
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
-  vector(size_type __n, const value_type& __x, const allocator_type& __a)
-      : __end_cap_(nullptr, __a) {
-    if (__n > 0) {
-      __vallocate(__n);
-      __construct_at_end(__n, __x);
-    }
-  }
+  vector(size_type __n, const value_type& __x, const allocator_type& __a);
 
   template <class _InputIterator,
             __enable_if_t<__has_exactly_input_iterator_category<_InputIterator>::value &&
@@ -1165,6 +1159,19 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 vector<_Tp, _Allocator>::vector(size_type __n, con
   __guard.__complete();
 }
 
+template <class _Tp, class _Allocator>
+template <__enable_if_t<__is_allocator<_Allocator>::value, int>>
+_LIBCPP_CONSTEXPR_SINCE_CXX20
+vector<_Tp, _Allocator>::vector(size_type __n, const value_type& __x, const allocator_type& __a)
+    : __end_cap_(nullptr, __a) {
+  auto __guard = std::__make_exception_guard(__destroy_vector(*this));
+  if (__n > 0) {
+    __vallocate(__n);
+    __construct_at_end(__n, __x);
+  }
+  __guard.__complete();
+}
+
 template <class _Tp, class _Allocator>
 template <class _InputIterator,
           __enable_if_t<__has_exactly_input_iterator_category<_InputIterator>::value &&

[philnik777]

Please don't move the code outside the class definition. We try to move away from that.

[mtezych]

OK, so you would prefer me to define all constructors filling std::vector<> with the same elements
inside the std::vector<> class template?

Notice that, you forgot to add the exception guard in the commit 8ff4d21,
most likely because only the vector(count, value, allocator) constructor
was defined inside the std::vector<> class template.

[philnik777]

I'd prefer to keep the bug fix and moving code separate. If you want to move the code, feel free to open a new PR. The code being defined in two different places being the reason seems like wild speculation to me. I'm pretty sure I went through the constructors on cppreference. Much more importantly: please add a test case for your change. Since you referenced my patch to fix most places, I think you should have a good idea of what to do. If you need any help feel free to ask here or on discord.

[mtezych]

Sure, I've created new pull request #82068 with the change that
inlines the remaining constructors filling vector with the same value.

I hope that placing those constructors physically next to each other
will make code better, since they all have nearly identical implementation,
which needs to be kept in sync.

---

Regarding the unit-tests, yes, of course, I agree that all changes fixing bugs
should also introduce tests verifying that these bugs were indeed fixed and will never happen again.

I even tried to extend your tests //libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp,
by adding test cases covering the vector(count, value, allocator) constructor.

However, these exception safety tests covering constructors of std::vector<> are so full of problems,
that I didn't even know where to start in order to fix them. In no particular order:

• The correctness of all test cases is verified only by calling check_new_delete_called(),
  which only checks whether operators new and delete were called equal number of times.
  Sure, this can to some extent verify whether vector's data buffer was allocated and freed correctly,
  but it definitely does not verify whether the vector's elements have been constructed and destructed correctly.

• Some of test cases, which supposed to verify whether a constructor of std::vector<> behaves correctly,
  when an exception is thrown while copying the allocator parameter, are in fact, not correct,
  because the Allocator class is defines non-throwing copy constructor:

  • Throw in vector(size_type, const allocator_type&) from allocator
  • Throw in vector(InputIterator, InputIterator, const allocator_type&) from allocator
  • Throw in vector(ForwardIterator, ForwardIterator, const allocator_type&) from allocator

• All test cases verifying correctness of std::vector constructors in situations,
  when an exception is thrown during construction of an element, are operating on single element vectors.
  This is a problem, because such test cases cannot verify whether destructors of already successfully created elements
  will indeed be invoked, when not all elements could have been successfully created.

• The ThrowingT class has reference semantics, since it holds a pointer to a counter,
  which determines when to throw an exception. I think that changing this class to represent a value type
  would make these unit-tests much easier to understand and extend in the future.

• In some test cases std::vector<> instances are named get_alloc. What's that about?

  • std::vector<ThrowingT> get_alloc(1);
  • std::vector<ThrowingT> get_alloc(1, v);
  • AllocVec get_alloc(0, alloc);

• Two test cases are available only, when they are compiled in C++11 mode,
  and yet C++11 features are used freely, without any #ifdefs.
  Which minimal version of the C++ standard can I assume?

  • #if TEST_STD_VER >= 11
  • using AllocVec = std::vector<int, Allocator<int> >;

In my opinion the above issues need to be resolved first, then the test cases covering this bug fix can be added.

I am perfectly happy to do so, but this will require rewriting almost the whole exceptions.pass.cpp source file,
so I don't want to do it without asking you first whether this is indeed the desired direction.

[philnik777]

As the comment in the test says, it's there to make sure that throwing in the constructors doesn't leak memory. It was never meant to check that all the correct destructors were run. If you want to take up the effort to improve the test coverage of exceptions in the vector constructors (and maybe other containers?) that would be great. I 100% agree that we need a lot better coverage in that area. AFAIK before #80558 we didn't have any coverage for destructing all the elements properly.

Regarding what exactly to do, I think it would be good to first fix exceptions.pass.cpp by adding assert(false) to make sure there are actually exceptions that are caught and fix any problems. Maybe also rename it to something that makes it clear the tests are only about memory leaks. After that I'd add a new test to make sure all the elements are destroyed correctly.

You can use any extensions that Clang provides in C++03. initializer_lists aren't provided as an extension in C++11, so the tests have to be guarded behind TEST_STD_VER >= 11.

[ldionne]

@mtezych Do you still have interest for finishing this patch?

[ldionne]

This is going to have to slip to the next release.