Skip to content

[libc++] Simplify vector<bool>::__construct_at_end (Reopend) #119632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 29, 2025
Merged

[libc++] Simplify vector<bool>::__construct_at_end (Reopend) #119632

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9ffbc8
Simplify vector<bool>::__construct_at_end
winner245 Dec 11, 2024
1b65294
Add _LIBCPP_ASSERT_INTERNAL
winner245 Jan 14, 2025
acc1a87
Add explicit precondition check
winner245 Jan 20, 2025
c8a5027
Remove unnecessary if condition and comment
winner245 Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 10 additions & 17 deletions libcxx/include/__vector/vector_bool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,36 +552,29 @@ vector<bool, _Allocator>::__recommend(size_type __new_size) const {
}

// Default constructs __n objects starting at __end_
// Precondition: __n > 0
// Precondition: size() + __n <= capacity()
// Postcondition: size() == size() + __n
template <class _Allocator>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
vector<bool, _Allocator>::__construct_at_end(size_type __n, bool __x) {
size_type __old_size = this->__size_;
_LIBCPP_ASSERT_INTERNAL(
capacity() >= size() + __n, "vector<bool>::__construct_at_end called with insufficient capacity");
std::fill_n(end(), __n, __x);
this->__size_ += __n;
if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
if (this->__size_ <= __bits_per_word)
this->__begin_[0] = __storage_type(0);
else
this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}
std::fill_n(__make_iter(__old_size), __n, __x);
if (end().__ctz_ != 0) // Ensure uninitialized leading bits in the last word are set to zero
std::fill_n(end(), __bits_per_word - end().__ctz_, 0);
Comment on lines +564 to +565
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures that the unused leading bits in the last word are initialized, which serves the same purpose as the pre-initialization logic in the previous implementation:

llvm-project/libcxx/include/__vector/vector_bool.h

Lines 554 to 561 in 743c84b

size_type __old_size = this->__size_;
this->__size_ += __n;
if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
if (this->__size_ <= __bits_per_word)
this->__begin_[0] = __storage_type(0);
else
this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}

Without this, we would have a Msan CI failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how is the previous fill_n not reading uninitialized elements? It has to load the bits of the last word after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In constant evaluation, we have no uninitialized bits at all because __vallocate guarantees that the allocated memory is default-constructed. In non-constant evaluation, we effectively only reads from bits within the bounds of vector<bool> since we utilize bit masks. So in this case, the purpose of the initialization is to make sanitizer tools happy. According to my test, sanitizers do not complain as long as the unused bits in the last word are initialized to 0 within __construct_at_end, regardless of whether this occurs before or after the initialization of other words. However, without the initialization of unused bits, subsequent read from vector<bool> would trigger a Msan CI failure.

}

template <class _Allocator>
template <class _InputIterator, class _Sentinel>
_LIBCPP_CONSTEXPR_SINCE_CXX20 void
vector<bool, _Allocator>::__construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test for this function where _InputIterator isn't a vector<bool> iterator? e.g. bool*. I don't see how the elements are initialized which makes me think that we may read from uninitialized memory when the __bit_iterator optimization in std::copy doesn't kick in. I don't think this is a problem in reality, since we're setting the bits one-by-one, but could cause some sanitizer to complain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern here. That's why I explicitly check if we have uninitialized bits, which could only exist in the last partial word. Previously, we pre-initialize the entire last word, whether it is a partial word or not. This patch implements it in a more compact form which initializes the bits to zero only when the last word is a partial word.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have basically the same question here: How does the __copy avoid the uninitialized read?

size_type __old_size = this->__size_;
_LIBCPP_ASSERT_INTERNAL(
capacity() >= size() + __n, "vector<bool>::__construct_at_end called with insufficient capacity");
std::__copy(std::move(__first), std::move(__last), end());
this->__size_ += __n;
if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
if (this->__size_ <= __bits_per_word)
this->__begin_[0] = __storage_type(0);
else
this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}
std::__copy(std::move(__first), std::move(__last), __make_iter(__old_size));
if (end().__ctz_ != 0) // Ensure uninitialized leading bits in the last word are set to zero
std::fill_n(end(), __bits_per_word - end().__ctz_, 0);
}

template <class _Allocator>
Expand Down
Loading