[libc++] Add exception guard for vector<bool>::__init_with_sentinel #115491

winner245 · 2024-11-08T14:42:51Z

I think this was missing from earlier PRs.

llvmbot · 2024-11-08T14:43:23Z

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

I think this was missing from earlier PRs.

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

1 Files Affected:

(modified) libcxx/include/__vector/vector_bool.h (+6-12)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 462095fd07acf7..4612cb96e67659 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -398,18 +398,12 @@ class _LIBCPP_TEMPLATE_VIS vector<bool, _Allocator> {
   template <class _InputIterator, class _Sentinel>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
   __init_with_sentinel(_InputIterator __first, _Sentinel __last) {
-#if _LIBCPP_HAS_EXCEPTIONS
-    try {
-#endif // _LIBCPP_HAS_EXCEPTIONS
-      for (; __first != __last; ++__first)
-        push_back(*__first);
-#if _LIBCPP_HAS_EXCEPTIONS
-    } catch (...) {
-      if (__begin_ != nullptr)
-        __storage_traits::deallocate(__alloc(), __begin_, __cap());
-      throw;
-    }
-#endif // _LIBCPP_HAS_EXCEPTIONS
+    auto __guard = std::__make_exception_guard(__destroy_vector(*this));
+
+    for (; __first != __last; ++__first)
+      push_back(*__first);
+
+    __guard.__complete();
   }
 
   template <class _Iterator, class _Sentinel>

ldionne

LGTM assuming this is NFC like it seems to be.

libcxx/include/__vector/vector_bool.h

ldionne · 2024-11-26T20:04:13Z

libcxx/test/support/exception_test_helpers.h

@@ -0,0 +1,87 @@
+//===----------------------------------------------------------------------===//


We have several helper classes in the test suite that attempt to provide reusable functionality like these ones. In practice, this often doesn't work really well because these classes are closely tied to the way they're used.

I spent some time trying to think about ways to make the classes below more "general purpose" so that it would make sense to actually reuse them in other tests (besides the ones for std::vector<T>) and I couldn't come up with something good. As it stands, I think it's unlikely that e.g. throwing_allocator would really be reused much outside of the std::vector classes (let me know if you disagree).

If you agree with the above, then I think it would be better to avoid putting these classes inside support/ to avoid giving the impression that they are general purpose. Instead, we can put them in e.g. libcxx/test/std/containers/sequences/vector/common.h.

Thank you for your suggestion. Yes, these classes, e.g., throwing_allocator, throwing_iterator were originally only used by the primary template std::vector<T, Alloc> before I added new tests for the vector<bool> specialization. During this PR, I noticed that we didn't have exception tests for vector<bool>, so I added these tests for the ctors of vector<bool>. Then I realized that the extracted classes such as throwing_allocator, throwing_iterator are now used by both the primary std::vector<T, Alloc> and the specialization vector<bool>.

I agree with your that these classes do not belong to support/ which targets more general purposes. Your suggestion libcxx/test/std/containers/sequences/vector/common.h sounds good to me.

ldionne · 2024-11-26T20:15:27Z

libcxx/test/support/exception_test_helpers.h

+template <class T>
+struct throwing_allocator {
+  using value_type      = T;
+  using is_always_equal = std::false_type;


This isn't telling the truth -- why? This would need a comment.

This class was named Allocator<T> which sounds a general allocator class with no indication of whether it may throw exception. Renaming it from Allocator to throwing_allocator makes its purpose to throw exceptions more explicit.

Thank you for this comment, which motivated me to think more deeply about the issue. I just realized that I must have misunderstood the question initially. Now I believe the question pertains to the line using is_always_equal = std::false_type;, which appears counter-intuitive for a stateless allocator class.

This line was in the original class Allocator before my refactor, my best guess is that the original author intended to use this class to test the exception safety for the constructor vector(vector&&, const allocator_type&) in vector/vector.cons/exceptions.pass.cpp:

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)); } catch (int) { } check_new_delete_called();

This test aims to verify the exception safety for the constructor while performing an O(n) element-wise move (rather than an O(1) resource stealing operation), which requires the instances of the Allocator class to compare unequal. However, merely setting Allocator::is_always_equal::value to false does not ensure that the instances of this class are unequal, which renders the above test actually testing nothing, as you also confirmed here.

Beyond this scenario, I couldn't think of any reason why we might need this member type. I believe we can safely remove it. Would you suggest I remove it in this PR or in my next one while rebasing?

The above test has been replaced by the following test:

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();

This test now uses test_allocator<T>, which checks equality between the allocator instances based on the data_ field. In the above test, the data_ fields 1 and 2 make the two allocators unequal, thus ensuring element-wise move.

The original test used a stateless allocator type Allocator (now renamed to throwing_allocator), which is not suitable for the above purpose. Also, the fact that a stateless allocator type has a false is_always_equal value sounds a bit counter-intuitive to me. That's why I removed the member type in my later PR. Now allocator_traits<throwing_allocator>::is_always_equal becomes true and this is consistence with its operator== which always returns true.

ldionne · 2024-11-26T20:24:22Z

libcxx/test/support/exception_test_helpers.h

+
+  bool throw_on_copy_ = false;
+
+  throwing_allocator(bool throw_on_ctor = true, bool throw_on_copy = false) : throw_on_copy_(throw_on_copy) {


Suggested change

throwing_allocator(bool throw_on_ctor = true, bool throw_on_copy = false) : throw_on_copy_(throw_on_copy) {

explicit throwing_allocator(bool throw_on_ctor, bool throw_on_copy) : throw_on_copy_(throw_on_copy) {

Let's remove the defaults. When we construct, let's use throwing_allocator alloc(/* throw_on_ctor */true, /* throw_on_copy */false); (for example), to make this more explicit.

ldionne · 2024-11-26T20:25:03Z

libcxx/test/support/exception_test_helpers.h

+#define EXCEPTION_TEST_HELPERS_H
+
+#include "count_new.h"
+


Missing includes <type_traits>, <cstddef>, <memory> and <cassert>.

ldionne · 2024-11-26T20:25:47Z

libcxx/test/support/exception_test_helpers.h

+  assert(globalMemCounter.aligned_new_array_called == globalMemCounter.aligned_delete_array_called);
+}
+
+#endif // EXCEPTION_TEST_HELPERS_H


Missing newline at the end of the file.

ldionne · 2024-11-26T20:27:08Z

libcxx/test/std/containers/sequences/vector.bool/exceptions.pass.cpp

@@ -0,0 +1,130 @@
+//===----------------------------------------------------------------------===//


Let's rename this file to ctor.exceptions.pass.cpp since we're only testing constructors.

Actually, writing this comment made me realize that we already have libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp. What's up with that test? We should merge this new test into that existing test instead. Do you have thoughts about that?

Oops, I should have noticed this file earlier—it would have simplified my PR.
I just quickly reviewed this file libcxx/test/std/containers/sequences/vector.bool/ctor_exceptions.pass.cpp and found it has similarly flaw as in the exception test for std::vector: the same Allocator class used in ctor_exceptions.pass.cpp does not trigger exception at all, making some tests checking for nothing for std::vector<bool>. Also, this test file also provided duplicate definitions for the Allocator and Iterator classes. I believe we still need to refactor these common classes into a file such as libcxx/test/std/containers/sequences/vector/common.h, as you suggested earlier. I will work on this first.

ldionne · 2024-11-26T21:01:28Z

For information, I just merged #116449 so you might want to rebase on top of the latest main.

github-actions · 2024-11-27T00:36:14Z

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

winner245 · 2024-11-27T13:40:08Z

I have done all the changes. Here is a brief summary for your information:

The common classes for vector and vector<bool> tests are now organized in vector/common.h, with some necessary definition improvements and meaningful name changes: Alloctor > throwing_allocator, Iterator > throwing_iterator. I think this header will become useful in the future if we need exception test coverage for other non-ctor operations, which will be discussed in a reply to your other question.
The test file vector.bool/ctor_exceptions.pass.cpp has been augmented with 5 additional tests:
- 3 of which covering exception tests for the copy/move-ctors with a specified allocator;
- 2 of which covering exception tests for other ctors with/without a specified allocator.

I think it is ready to merge. Once it is merged, I will proceed to rebase my other PRs.

ldionne

Thank you, this is great!

libcxx/test/std/containers/sequences/vector/common.h

winner245 requested a review from a team as a code owner November 8, 2024 14:42

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 8, 2024

ldionne approved these changes Nov 11, 2024

View reviewed changes

libcxx/include/__vector/vector_bool.h Show resolved Hide resolved

winner245 force-pushed the winner245/vector_bool_exception_guard branch 5 times, most recently from ca65517 to 5158f0c Compare November 19, 2024 04:35

winner245 force-pushed the winner245/vector_bool_exception_guard branch 3 times, most recently from fc8b88f to 1b6d161 Compare November 26, 2024 02:51

ldionne requested changes Nov 26, 2024

View reviewed changes

winner245 force-pushed the winner245/vector_bool_exception_guard branch from 1b6d161 to bfcd237 Compare November 27, 2024 00:32

winner245 force-pushed the winner245/vector_bool_exception_guard branch 2 times, most recently from 7c1d753 to 31d52fe Compare November 27, 2024 01:16

winner245 added 7 commits November 27, 2024 17:55

Add exception tests for vector<bool>

2220361

Add exception guard for vector<bool>::__init_with_sentinel

5f127ed

Run clang-format

c8010fd

Update header guard to match the header name

f14822a

Fix compatibility

d1edb2e

Refactor in a better way

2cb7028

Use more complex header guard name for common.h

d43d704

winner245 force-pushed the winner245/vector_bool_exception_guard branch from 31d52fe to d43d704 Compare November 27, 2024 22:56

ldionne approved these changes Nov 28, 2024

View reviewed changes

libcxx/test/std/containers/sequences/vector/common.h Outdated Show resolved Hide resolved

Update libcxx/test/std/containers/sequences/vector/common.h

8e12408

ldionne merged commit c5cd1e9 into llvm:main Nov 28, 2024
10 of 12 checks passed

winner245 deleted the winner245/vector_bool_exception_guard branch November 28, 2024 15:46

winner245 mentioned this pull request Nov 28, 2024

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

Merged

		@@ -0,0 +1,87 @@
		//===----------------------------------------------------------------------===//


		bool throw_on_copy_ = false;

		throwing_allocator(bool throw_on_ctor = true, bool throw_on_copy = false) : throw_on_copy_(throw_on_copy) {

	throwing_allocator(bool throw_on_ctor = true, bool throw_on_copy = false) : throw_on_copy_(throw_on_copy) {
	explicit throwing_allocator(bool throw_on_ctor, bool throw_on_copy) : throw_on_copy_(throw_on_copy) {

		@@ -0,0 +1,130 @@
		//===----------------------------------------------------------------------===//

		#define EXCEPTION_TEST_HELPERS_H

		#include "count_new.h"

[libc++] Add exception guard for vector<bool>::__init_with_sentinel #115491

[libc++] Add exception guard for vector<bool>::__init_with_sentinel #115491

Uh oh!

Conversation

winner245 commented Nov 8, 2024

Uh oh!

llvmbot commented Nov 8, 2024

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winner245 Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winner245 Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winner245 Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne commented Nov 26, 2024

Uh oh!

github-actions bot commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winner245 commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

winner245 Nov 27, 2024 •

edited

Loading

winner245 Nov 28, 2024 •

edited

Loading

winner245 Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024 •

edited

Loading

winner245 commented Nov 27, 2024 •

edited

Loading