From de585eaa98a94a4f0f193623343aacb67149af62 Mon Sep 17 00:00:00 2001 From: Peng Liu Date: Thu, 9 Jan 2025 21:58:49 -0500 Subject: [PATCH 1/4] Fix UB in bitwise logic of std/ranges::fill/fill_n algorithms --- libcxx/include/__algorithm/fill_n.h | 4 +- libcxx/include/__fwd/bit_reference.h | 18 ++ .../alg.fill/fill.pass.cpp | 35 +++ .../alg.fill/fill_n.pass.cpp | 220 ++++++++++-------- .../alg.fill/ranges.fill.pass.cpp | 52 ++++- .../alg.fill/ranges.fill_n.pass.cpp | 44 +++- libcxx/test/support/sized_allocator.h | 58 +++++ 7 files changed, 321 insertions(+), 110 deletions(-) create mode 100644 libcxx/test/support/sized_allocator.h diff --git a/libcxx/include/__algorithm/fill_n.h b/libcxx/include/__algorithm/fill_n.h index a7e01c45b9222..bbdc70c83e54f 100644 --- a/libcxx/include/__algorithm/fill_n.h +++ b/libcxx/include/__algorithm/fill_n.h @@ -41,7 +41,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ if (__first.__ctz_ != 0) { __storage_type __clz_f = static_cast<__storage_type>(__bits_per_word - __first.__ctz_); __storage_type __dn = std::min(__clz_f, __n); - __storage_type __m = (~__storage_type(0) << __first.__ctz_) & (~__storage_type(0) >> (__clz_f - __dn)); + __storage_type __m = std::__middle_mask<__storage_type>(__first.__ctz_, __clz_f - __dn); if (_FillVal) *__first.__seg_ |= __m; else @@ -56,7 +56,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ // do last partial word if (__n > 0) { __first.__seg_ += __nw; - __storage_type __m = ~__storage_type(0) >> (__bits_per_word - __n); + __storage_type __m = std::__trailing_mask<__storage_type>(__bits_per_word - __n); if (_FillVal) *__first.__seg_ |= __m; else diff --git a/libcxx/include/__fwd/bit_reference.h b/libcxx/include/__fwd/bit_reference.h index 30462b6ce4c92..3dce495c6626c 100644 --- a/libcxx/include/__fwd/bit_reference.h +++ b/libcxx/include/__fwd/bit_reference.h @@ -10,6 +10,8 @@ #define _LIBCPP___FWD_BIT_REFERENCE_H #include <__config> +#include <__type_traits/enable_if.h> +#include <__type_traits/is_unsigned.h> #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) # pragma GCC system_header @@ -23,6 +25,22 @@ class __bit_iterator; template struct __size_difference_type_traits; +template ::value, int> = 0> +_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __leading_mask(unsigned __shift) { + return static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __shift); +} + +template ::value, int> = 0> +_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __trailing_mask(unsigned __shift) { + return static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __shift); +} + +template ::value, int> = 0> +_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __middle_mask(unsigned __lshift, unsigned __rshift) { + return static_cast<_StorageType>( + std::__leading_mask<_StorageType>(__lshift) & std::__trailing_mask<_StorageType>(__rshift)); +} + _LIBCPP_END_NAMESPACE_STD #endif // _LIBCPP___FWD_BIT_REFERENCE_H diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp index 619dc7242a366..a1797fab6cc14 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp @@ -19,6 +19,7 @@ #include #include +#include "sized_allocator.h" #include "test_macros.h" #include "test_iterators.h" @@ -46,6 +47,37 @@ struct Test { } }; +TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { + { + using Alloc = sized_allocator; + std::vector in(100, false, Alloc(1)); + std::vector expected(100, true, Alloc(1)); + std::fill(in.begin(), in.end(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill(in.begin(), in.end(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill(in.begin(), in.end(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill(in.begin(), in.end(), true); + assert(in == expected); + } +} + TEST_CONSTEXPR_CXX20 bool test() { types::for_each(types::forward_iterator_list(), Test()); types::for_each(types::forward_iterator_list(), Test()); @@ -93,6 +125,9 @@ TEST_CONSTEXPR_CXX20 bool test() { assert(in == expected); } } + + test_bititer_with_custom_sized_types(); + return true; } diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp index 7d6770de702bf..582889dbb21d1 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp @@ -15,159 +15,175 @@ #include #include +#include +#include "sized_allocator.h" #include "test_macros.h" #include "test_iterators.h" #include "user_defined_integral.h" #if TEST_STD_VER > 17 TEST_CONSTEXPR bool test_constexpr() { - const std::size_t N = 5; - int ib[] = {0, 0, 0, 0, 0, 0}; // one bigger than N - - auto it = std::fill_n(std::begin(ib), N, 5); - return it == (std::begin(ib) + N) - && std::all_of(std::begin(ib), it, [](int a) {return a == 5; }) - && *it == 0 // don't overwrite the last value in the output array - ; - } + const std::size_t N = 5; + int ib[] = {0, 0, 0, 0, 0, 0}; // one bigger than N + + auto it = std::fill_n(std::begin(ib), N, 5); + return it == (std::begin(ib) + N) && std::all_of(std::begin(ib), it, [](int a) { return a == 5; }) && + *it == 0 // don't overwrite the last value in the output array + ; +} #endif typedef UserDefinedIntegral UDI; template -void -test_char() -{ - char a[4] = {}; - Iter it = std::fill_n(Iter(a), UDI(4), char(1)); - assert(base(it) == a + 4); - assert(a[0] == 1); - assert(a[1] == 1); - assert(a[2] == 1); - assert(a[3] == 1); +void test_char() { + char a[4] = {}; + Iter it = std::fill_n(Iter(a), UDI(4), char(1)); + assert(base(it) == a + 4); + assert(a[0] == 1); + assert(a[1] == 1); + assert(a[2] == 1); + assert(a[3] == 1); } template -void -test_int() -{ - int a[4] = {}; - Iter it = std::fill_n(Iter(a), UDI(4), 1); - assert(base(it) == a + 4); - assert(a[0] == 1); - assert(a[1] == 1); - assert(a[2] == 1); - assert(a[3] == 1); +void test_int() { + int a[4] = {}; + Iter it = std::fill_n(Iter(a), UDI(4), 1); + assert(base(it) == a + 4); + assert(a[0] == 1); + assert(a[1] == 1); + assert(a[2] == 1); + assert(a[3] == 1); } -void -test_int_array() -{ - int a[4] = {}; - assert(std::fill_n(a, UDI(4), static_cast(1)) == a + 4); - assert(a[0] == 1); - assert(a[1] == 1); - assert(a[2] == 1); - assert(a[3] == 1); +void test_int_array() { + int a[4] = {}; + assert(std::fill_n(a, UDI(4), static_cast(1)) == a + 4); + assert(a[0] == 1); + assert(a[1] == 1); + assert(a[2] == 1); + assert(a[3] == 1); } struct source { - source() : i(0) { } + source() : i(0) {} - operator int() const { return i++; } - mutable int i; + operator int() const { return i++; } + mutable int i; }; -void -test_int_array_struct_source() -{ - int a[4] = {}; - assert(std::fill_n(a, UDI(4), source()) == a + 4); - assert(a[0] == 0); - assert(a[1] == 1); - assert(a[2] == 2); - assert(a[3] == 3); +void test_int_array_struct_source() { + int a[4] = {}; + assert(std::fill_n(a, UDI(4), source()) == a + 4); + assert(a[0] == 0); + assert(a[1] == 1); + assert(a[2] == 2); + assert(a[3] == 3); } struct test1 { - test1() : c(0) { } - test1(char xc) : c(xc + 1) { } - char c; + test1() : c(0) {} + test1(char xc) : c(xc + 1) {} + char c; }; -void -test_struct_array() -{ - test1 test1a[4] = {}; - assert(std::fill_n(test1a, UDI(4), static_cast(10)) == test1a + 4); - assert(test1a[0].c == 11); - assert(test1a[1].c == 11); - assert(test1a[2].c == 11); - assert(test1a[3].c == 11); +void test_struct_array() { + test1 test1a[4] = {}; + assert(std::fill_n(test1a, UDI(4), static_cast(10)) == test1a + 4); + assert(test1a[0].c == 11); + assert(test1a[1].c == 11); + assert(test1a[2].c == 11); + assert(test1a[3].c == 11); } -class A -{ - char a_; +class A { + char a_; + public: - A() {} - explicit A(char a) : a_(a) {} - operator unsigned char() const {return 'b';} + A() {} + explicit A(char a) : a_(a) {} + operator unsigned char() const { return 'b'; } - friend bool operator==(const A& x, const A& y) - {return x.a_ == y.a_;} + friend bool operator==(const A& x, const A& y) { return x.a_ == y.a_; } }; -void -test5() -{ - A a[3]; - assert(std::fill_n(&a[0], UDI(3), A('a')) == a+3); - assert(a[0] == A('a')); - assert(a[1] == A('a')); - assert(a[2] == A('a')); +void test5() { + A a[3]; + assert(std::fill_n(&a[0], UDI(3), A('a')) == a + 3); + assert(a[0] == A('a')); + assert(a[1] == A('a')); + assert(a[2] == A('a')); } -struct Storage -{ - union - { +struct Storage { + union { unsigned char a; unsigned char b; }; }; -void test6() -{ +void test6() { Storage foo[5]; std::fill_n(&foo[0], UDI(5), Storage()); } +TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { + { + using Alloc = sized_allocator; + std::vector in(100, false, Alloc(1)); + std::vector expected(100, true, Alloc(1)); + std::fill_n(in.begin(), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill_n(in.begin(), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill_n(in.begin(), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::fill_n(in.begin(), in.size(), true); + assert(in == expected); + } +} + +int main(int, char**) { + test_char >(); + test_char >(); + test_char >(); + test_char >(); + test_char(); -int main(int, char**) -{ - test_char >(); - test_char >(); - test_char >(); - test_char >(); - test_char(); + test_int >(); + test_int >(); + test_int >(); + test_int >(); + test_int(); - test_int >(); - test_int >(); - test_int >(); - test_int >(); - test_int(); + test_int_array(); + test_int_array_struct_source(); + test_struct_array(); - test_int_array(); - test_int_array_struct_source(); - test_struct_array(); + test5(); + test6(); - test5(); - test6(); + test_bititer_with_custom_sized_types(); #if TEST_STD_VER > 17 - static_assert(test_constexpr()); + static_assert(test_constexpr()); #endif return 0; diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp index 5dc375e0e8dc0..870f05bc55514 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp @@ -20,9 +20,12 @@ #include #include #include +#include +#include "sized_allocator.h" #include "almost_satisfies_types.h" #include "test_iterators.h" +#include "test_macros.h" template > concept HasFillIt = requires(Iter iter, Sent sent) { std::ranges::fill(iter, sent, int{}); }; @@ -53,7 +56,7 @@ constexpr void test_iterators() { } { int a[3]; - auto range = std::ranges::subrange(It(a), Sent(It(a + 3))); + auto range = std::ranges::subrange(It(a), Sent(It(a + 3))); std::same_as auto ret = std::ranges::fill(range, 1); assert(std::all_of(a, a + 3, [](int i) { return i == 1; })); assert(base(ret) == a + 3); @@ -69,12 +72,47 @@ constexpr void test_iterators() { { std::array a; auto range = std::ranges::subrange(It(a.data()), Sent(It(a.data()))); - auto ret = std::ranges::fill(range, 1); + auto ret = std::ranges::fill(range, 1); assert(base(ret) == a.data()); } } } +// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy the +// `std::indirectly_writable` concept when used with `vector`, which is only met since C++23. +#if TEST_STD_VER >= 23 +TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { + { + using Alloc = sized_allocator; + std::vector in(100, false, Alloc(1)); + std::vector expected(100, true, Alloc(1)); + std::ranges::fill(in, true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill(in, true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill(in, true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill(in, true); + assert(in == expected); + } +} +#endif + constexpr bool test() { test_iterators, sentinel_wrapper>>(); test_iterators, sentinel_wrapper>>(); @@ -94,19 +132,19 @@ constexpr bool test() { }; { S a[5]; - std::ranges::fill(a, a + 5, S {true}); + std::ranges::fill(a, a + 5, S{true}); assert(std::all_of(a, a + 5, [](S& s) { return s.copied; })); } { S a[5]; - std::ranges::fill(a, S {true}); + std::ranges::fill(a, S{true}); assert(std::all_of(a, a + 5, [](S& s) { return s.copied; })); } } { // check that std::ranges::dangling is returned [[maybe_unused]] std::same_as decltype(auto) ret = - std::ranges::fill(std::array {}, 1); + std::ranges::fill(std::array{}, 1); } { // check that std::ranges::dangling isn't returned with a borrowing range @@ -131,6 +169,10 @@ constexpr bool test() { } } +#if TEST_STD_VER >= 23 + test_bititer_with_custom_sized_types(); +#endif + return true; } diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp index 10ff385d47428..fc3289772b886 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp @@ -18,9 +18,12 @@ #include #include #include +#include +#include "sized_allocator.h" #include "almost_satisfies_types.h" #include "test_iterators.h" +#include "test_macros.h" template concept HasFillN = requires(Iter iter) { std::ranges::fill_n(iter, int{}, int{}); }; @@ -48,6 +51,41 @@ constexpr void test_iterators() { } } +// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy the +// `std::indirectly_writable` concept when used with `vector`, which is only met since C++23. +#if TEST_STD_VER >= 23 +TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { + { + using Alloc = sized_allocator; + std::vector in(100, false, Alloc(1)); + std::vector expected(100, true, Alloc(1)); + std::ranges::fill_n(std::ranges::begin(in), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill_n(std::ranges::begin(in), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill_n(std::ranges::begin(in), in.size(), true); + assert(in == expected); + } + { + using Alloc = sized_allocator; + std::vector in(200, false, Alloc(1)); + std::vector expected(200, true, Alloc(1)); + std::ranges::fill_n(std::ranges::begin(in), in.size(), true); + assert(in == expected); + } +} +#endif + constexpr bool test() { test_iterators, sentinel_wrapper>>(); test_iterators, sentinel_wrapper>>(); @@ -68,7 +106,7 @@ constexpr bool test() { }; S a[5]; - std::ranges::fill_n(a, 5, S {}); + std::ranges::fill_n(a, 5, S{}); assert(std::all_of(a, a + 5, [](S& s) { return s.copied; })); } @@ -79,6 +117,10 @@ constexpr bool test() { assert(std::all_of(a.begin(), a.end(), [](auto& s) { return s == "long long string so no SSO"; })); } +#if TEST_STD_VER >= 23 + test_bititer_with_custom_sized_types(); +#endif + return true; } diff --git a/libcxx/test/support/sized_allocator.h b/libcxx/test/support/sized_allocator.h new file mode 100644 index 0000000000000..a877252e82962 --- /dev/null +++ b/libcxx/test/support/sized_allocator.h @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// 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 TEST_SUPPORT_SIZED_ALLOCATOR_H +#define TEST_SUPPORT_SIZED_ALLOCATOR_H + +#include +#include +#include +#include + +#include "test_macros.h" + +template +class sized_allocator { + template + friend class sized_allocator; + +public: + using value_type = T; + using size_type = SIZE_TYPE; + using difference_type = DIFF_TYPE; + using propagate_on_container_swap = std::true_type; + + TEST_CONSTEXPR_CXX20 explicit sized_allocator(int d = 0) : data_(d) {} + + template + TEST_CONSTEXPR_CXX20 sized_allocator(const sized_allocator& a) TEST_NOEXCEPT : data_(a.data_) {} + + TEST_CONSTEXPR_CXX20 T* allocate(size_type n) { + if (n > max_size()) + TEST_THROW(std::bad_array_new_length()); + return std::allocator().allocate(n); + } + + TEST_CONSTEXPR_CXX20 void deallocate(T* p, size_type n) TEST_NOEXCEPT { std::allocator().deallocate(p, n); } + + TEST_CONSTEXPR size_type max_size() const TEST_NOEXCEPT { + return std::numeric_limits::max() / sizeof(value_type); + } + +private: + int data_; + + TEST_CONSTEXPR friend bool operator==(const sized_allocator& a, const sized_allocator& b) { + return a.data_ == b.data_; + } + TEST_CONSTEXPR friend bool operator!=(const sized_allocator& a, const sized_allocator& b) { + return a.data_ != b.data_; + } +}; + +#endif From f9d9b4f6da29715d83ad327c5129d8a84f676805 Mon Sep 17 00:00:00 2001 From: Peng Liu Date: Wed, 15 Jan 2025 09:16:14 -0500 Subject: [PATCH 2/4] Address @ldionne's review comments --- libcxx/include/__algorithm/fill_n.h | 12 ++---------- libcxx/include/__bit_reference | 23 +++++++++++++++++++++++ libcxx/include/__fwd/bit_reference.h | 20 +++++--------------- libcxx/test/support/sized_allocator.h | 8 ++++---- libcxx/utils/libcxx/test/params.py | 1 + 5 files changed, 35 insertions(+), 29 deletions(-) diff --git a/libcxx/include/__algorithm/fill_n.h b/libcxx/include/__algorithm/fill_n.h index bbdc70c83e54f..a0141fafbc29c 100644 --- a/libcxx/include/__algorithm/fill_n.h +++ b/libcxx/include/__algorithm/fill_n.h @@ -41,11 +41,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ if (__first.__ctz_ != 0) { __storage_type __clz_f = static_cast<__storage_type>(__bits_per_word - __first.__ctz_); __storage_type __dn = std::min(__clz_f, __n); - __storage_type __m = std::__middle_mask<__storage_type>(__first.__ctz_, __clz_f - __dn); - if (_FillVal) - *__first.__seg_ |= __m; - else - *__first.__seg_ &= ~__m; + std::__fill_masked_range(std::__to_address(__first.__seg_), __first.__ctz_, __clz_f - __dn, _FillVal); __n -= __dn; ++__first.__seg_; } @@ -56,11 +52,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ // do last partial word if (__n > 0) { __first.__seg_ += __nw; - __storage_type __m = std::__trailing_mask<__storage_type>(__bits_per_word - __n); - if (_FillVal) - *__first.__seg_ |= __m; - else - *__first.__seg_ &= ~__m; + std::__fill_masked_range(std::__to_address(__first.__seg_), 0u, __bits_per_word - __n, _FillVal); } } diff --git a/libcxx/include/__bit_reference b/libcxx/include/__bit_reference index 67abb023122ed..2576a9a649bd6 100644 --- a/libcxx/include/__bit_reference +++ b/libcxx/include/__bit_reference @@ -12,6 +12,7 @@ #include <__algorithm/copy_n.h> #include <__algorithm/min.h> +#include <__assert> #include <__bit/countr.h> #include <__compare/ordering.h> #include <__config> @@ -22,9 +23,12 @@ #include <__memory/construct_at.h> #include <__memory/pointer_traits.h> #include <__type_traits/conditional.h> +#include <__type_traits/enable_if.h> #include <__type_traits/is_constant_evaluated.h> +#include <__type_traits/is_unsigned.h> #include <__type_traits/void_t.h> #include <__utility/swap.h> +#include #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) # pragma GCC system_header @@ -55,6 +59,25 @@ struct __size_difference_type_traits<_Cp, __void_t::element_type>::value, int> > +_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void +__fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val) { + using _StorageType = typename pointer_traits<_StoragePointer>::element_type; + _LIBCPP_ASSERT_VALID_INPUT_RANGE( + __ctz + __clz < sizeof(_StorageType) * CHAR_BIT, "__fill_masked_range called with invalid range"); + _StorageType __m = static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __ctz) & + static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __clz); + if (__fill_val) + *__word |= __m; + else + *__word &= static_cast<_StorageType>(~__m); +} + template ::value> class __bit_reference { using __storage_type _LIBCPP_NODEBUG = typename _Cp::__storage_type; diff --git a/libcxx/include/__fwd/bit_reference.h b/libcxx/include/__fwd/bit_reference.h index 3dce495c6626c..278f61bf875a7 100644 --- a/libcxx/include/__fwd/bit_reference.h +++ b/libcxx/include/__fwd/bit_reference.h @@ -10,6 +10,7 @@ #define _LIBCPP___FWD_BIT_REFERENCE_H #include <__config> +#include <__memory/pointer_traits.h> #include <__type_traits/enable_if.h> #include <__type_traits/is_unsigned.h> @@ -25,21 +26,10 @@ class __bit_iterator; template struct __size_difference_type_traits; -template ::value, int> = 0> -_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __leading_mask(unsigned __shift) { - return static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __shift); -} - -template ::value, int> = 0> -_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __trailing_mask(unsigned __shift) { - return static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __shift); -} - -template ::value, int> = 0> -_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __middle_mask(unsigned __lshift, unsigned __rshift) { - return static_cast<_StorageType>( - std::__leading_mask<_StorageType>(__lshift) & std::__trailing_mask<_StorageType>(__rshift)); -} +template ::element_type>::value, int> = 0> +_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void +__fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val); _LIBCPP_END_NAMESPACE_STD diff --git a/libcxx/test/support/sized_allocator.h b/libcxx/test/support/sized_allocator.h index a877252e82962..a9cec8ee28f94 100644 --- a/libcxx/test/support/sized_allocator.h +++ b/libcxx/test/support/sized_allocator.h @@ -16,15 +16,15 @@ #include "test_macros.h" -template +template class sized_allocator { template friend class sized_allocator; public: using value_type = T; - using size_type = SIZE_TYPE; - using difference_type = DIFF_TYPE; + using size_type = Size; + using difference_type = Difference; using propagate_on_container_swap = std::true_type; TEST_CONSTEXPR_CXX20 explicit sized_allocator(int d = 0) : data_(d) {} @@ -55,4 +55,4 @@ class sized_allocator { } }; -#endif +#endif // TEST_SUPPORT_SIZED_ALLOCATOR_H diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py index 8fd3872cd8cbb..7dba39bc5db04 100644 --- a/libcxx/utils/libcxx/test/params.py +++ b/libcxx/utils/libcxx/test/params.py @@ -31,6 +31,7 @@ "-Wno-reserved-module-identifier", '-Wdeprecated-copy', '-Wdeprecated-copy-dtor', + "-Wshift-negative-value", # GCC warns about places where we might want to add sized allocation/deallocation # functions, but we know better what we're doing/testing in the test suite. "-Wno-sized-deallocation", From a2d2f23689ab6f3e4e7547dc84079cb8b2c63704 Mon Sep 17 00:00:00 2001 From: Peng Liu Date: Mon, 20 Jan 2025 15:52:11 -0500 Subject: [PATCH 3/4] Switch order of __clz and __ctz --- libcxx/include/__algorithm/fill_n.h | 4 ++-- libcxx/include/__bit_reference | 8 ++++---- .../alg.fill/ranges.fill.pass.cpp | 8 ++++++-- .../alg.fill/ranges.fill_n.pass.cpp | 8 ++++++-- libcxx/test/support/sized_allocator.h | 2 ++ 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/libcxx/include/__algorithm/fill_n.h b/libcxx/include/__algorithm/fill_n.h index a0141fafbc29c..0da78e1f38c4c 100644 --- a/libcxx/include/__algorithm/fill_n.h +++ b/libcxx/include/__algorithm/fill_n.h @@ -41,7 +41,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ if (__first.__ctz_ != 0) { __storage_type __clz_f = static_cast<__storage_type>(__bits_per_word - __first.__ctz_); __storage_type __dn = std::min(__clz_f, __n); - std::__fill_masked_range(std::__to_address(__first.__seg_), __first.__ctz_, __clz_f - __dn, _FillVal); + std::__fill_masked_range(std::__to_address(__first.__seg_), __clz_f - __dn, __first.__ctz_, _FillVal); __n -= __dn; ++__first.__seg_; } @@ -52,7 +52,7 @@ __fill_n_bool(__bit_iterator<_Cp, false> __first, typename __size_difference_typ // do last partial word if (__n > 0) { __first.__seg_ += __nw; - std::__fill_masked_range(std::__to_address(__first.__seg_), 0u, __bits_per_word - __n, _FillVal); + std::__fill_masked_range(std::__to_address(__first.__seg_), __bits_per_word - __n, 0u, _FillVal); } } diff --git a/libcxx/include/__bit_reference b/libcxx/include/__bit_reference index 2576a9a649bd6..7da79932c6061 100644 --- a/libcxx/include/__bit_reference +++ b/libcxx/include/__bit_reference @@ -62,16 +62,16 @@ struct __size_difference_type_traits<_Cp, __void_t::element_type>::value, int> > _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void -__fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val) { +__fill_masked_range(_StoragePointer __word, unsigned __clz, unsigned __ctz, bool __fill_val) { using _StorageType = typename pointer_traits<_StoragePointer>::element_type; _LIBCPP_ASSERT_VALID_INPUT_RANGE( __ctz + __clz < sizeof(_StorageType) * CHAR_BIT, "__fill_masked_range called with invalid range"); - _StorageType __m = static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __ctz) & - static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __clz); + _StorageType __m = static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) >> __clz) & + static_cast<_StorageType>(static_cast<_StorageType>(~static_cast<_StorageType>(0)) << __ctz); if (__fill_val) *__word |= __m; else diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp index 870f05bc55514..95097c9c47fdf 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp @@ -78,8 +78,12 @@ constexpr void test_iterators() { } } -// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy the -// `std::indirectly_writable` concept when used with `vector`, which is only met since C++23. +// Make sure we behave properly with std::vector iterators with custom size types, see +// https://github.com/llvm/llvm-project/pull/122410. +// +// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy +// the `std::indirectly_writable` concept when used with `vector`, which is only +// satisfied since C++23. #if TEST_STD_VER >= 23 TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { { diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp index fc3289772b886..7b42050c2a161 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp @@ -51,8 +51,12 @@ constexpr void test_iterators() { } } -// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy the -// `std::indirectly_writable` concept when used with `vector`, which is only met since C++23. +// Make sure we behave properly with std::vector iterators with custom size types, see +// https://github.com/llvm/llvm-project/pull/122410. +// +// The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy +// the `std::indirectly_writable` concept when used with `vector`, which is only +// satisfied since C++23. #if TEST_STD_VER >= 23 TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { { diff --git a/libcxx/test/support/sized_allocator.h b/libcxx/test/support/sized_allocator.h index a9cec8ee28f94..8d52f5bf252c7 100644 --- a/libcxx/test/support/sized_allocator.h +++ b/libcxx/test/support/sized_allocator.h @@ -16,6 +16,8 @@ #include "test_macros.h" +// Allocator with a provided size_type and difference_type, used to test corner cases +// like arithmetic on Allocator::size_type in generic code. template class sized_allocator { template From 30a92747c85374c9fd66c955c23ebb3212973ab6 Mon Sep 17 00:00:00 2001 From: Peng Liu Date: Wed, 29 Jan 2025 15:17:23 -0500 Subject: [PATCH 4/4] Replace enable_if by static_assert --- libcxx/include/__bit_reference | 5 +++-- libcxx/include/__fwd/bit_reference.h | 3 +-- .../alg.modifying.operations/alg.fill/fill.pass.cpp | 2 ++ .../alg.modifying.operations/alg.fill/fill_n.pass.cpp | 2 ++ .../alg.modifying.operations/alg.fill/ranges.fill.pass.cpp | 4 ++-- .../alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp | 4 ++-- 6 files changed, 12 insertions(+), 8 deletions(-) diff --git a/libcxx/include/__bit_reference b/libcxx/include/__bit_reference index 7da79932c6061..35b507284125a 100644 --- a/libcxx/include/__bit_reference +++ b/libcxx/include/__bit_reference @@ -63,10 +63,11 @@ struct __size_difference_type_traits<_Cp, __void_t::element_type>::value, int> > +template _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __fill_masked_range(_StoragePointer __word, unsigned __clz, unsigned __ctz, bool __fill_val) { + static_assert(is_unsigned::element_type>::value, + "__fill_masked_range must be called with unsigned type"); using _StorageType = typename pointer_traits<_StoragePointer>::element_type; _LIBCPP_ASSERT_VALID_INPUT_RANGE( __ctz + __clz < sizeof(_StorageType) * CHAR_BIT, "__fill_masked_range called with invalid range"); diff --git a/libcxx/include/__fwd/bit_reference.h b/libcxx/include/__fwd/bit_reference.h index 278f61bf875a7..d65f043e89ad6 100644 --- a/libcxx/include/__fwd/bit_reference.h +++ b/libcxx/include/__fwd/bit_reference.h @@ -26,8 +26,7 @@ class __bit_iterator; template struct __size_difference_type_traits; -template ::element_type>::value, int> = 0> +template _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __fill_masked_range(_StoragePointer __word, unsigned __ctz, unsigned __clz, bool __fill_val); diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp index a1797fab6cc14..7656be73c14c6 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill.pass.cpp @@ -47,6 +47,8 @@ struct Test { } }; +// Make sure std::fill behaves properly with std::vector iterators with custom size types. +// See https://github.com/llvm/llvm-project/pull/122410. TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { { using Alloc = sized_allocator; diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp index 582889dbb21d1..3b67101a8b29e 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp @@ -129,6 +129,8 @@ void test6() { std::fill_n(&foo[0], UDI(5), Storage()); } +// Make sure std::fill_n behaves properly with std::vector iterators with custom size types. +// See https://github.com/llvm/llvm-project/pull/122410. TEST_CONSTEXPR_CXX20 void test_bititer_with_custom_sized_types() { { using Alloc = sized_allocator; diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp index 95097c9c47fdf..30dfdd5486f5b 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill.pass.cpp @@ -78,8 +78,8 @@ constexpr void test_iterators() { } } -// Make sure we behave properly with std::vector iterators with custom size types, see -// https://github.com/llvm/llvm-project/pull/122410. +// Make sure std::ranges::fill behaves properly with std::vector iterators with custom +// size types. See https://github.com/llvm/llvm-project/pull/122410. // // The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy // the `std::indirectly_writable` concept when used with `vector`, which is only diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp index 7b42050c2a161..ae70e7155f67f 100644 --- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp +++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/ranges.fill_n.pass.cpp @@ -51,8 +51,8 @@ constexpr void test_iterators() { } } -// Make sure we behave properly with std::vector iterators with custom size types, see -// https://github.com/llvm/llvm-project/pull/122410. +// Make sure std::ranges::fill_n behaves properly with std::vector iterators with custom +// size types. See https://github.com/llvm/llvm-project/pull/122410. // // The `ranges::{fill, fill_n}` algorithms require `vector::iterator` to satisfy // the `std::indirectly_writable` concept when used with `vector`, which is only