[libc++] Fix UB in bitwise logic of {std, ranges}::{fill, fill_n} algorithms #122410
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
3 times, most recently
from
2ecddb8 to
c84a156
Compare
winner245
changed the title
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
6 times, most recently
from
8030070 to
fa1fb7f
Compare
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesThis PR addresses an undefined behavior that arises when using the When assigning to a Here, the evaluation Since this UB typically results in a compilation warning, it can be difficult to detect. Running the new tests added in this PR without applying my fix will cause only the To reproduce this UB, we should enable the warning as an error with the following compilation flags in both Clang and GCC: int main() {
using __storage_type = unsigned short;
auto y = ~__storage_type(0) << 1;
(void)y;
}When compiling this code, Clang will produce the following error message (gcc exhibits a similar behavior): Solution: since integral promotions in bitwise arithmetic for small integral types are inevitable, to resolve this UB, we need to ensure that the result of each bitwise operation is explicitly cast back to the unsigned Additional Note: This UB was first observed while working on #119801 and has led to more serious ambiguous call bugs in Patch is 20.89 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/122410.diff 7 Files Affected:
diff --git a/libcxx/include/__algorithm/fill_n.h b/libcxx/include/__algorithm/fill_n.h
index 5069a72783f348..5bccf5c9bd0bee 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 _Cp::size_type __n) {
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 _Cp::size_type __n) {
// 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 237efb6db66429..c927e65747af08 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
@@ -20,6 +22,22 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0>
class __bit_iterator;
+template <class _StorageType, __enable_if_t<is_unsigned<_StorageType>::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 <class _StorageType, __enable_if_t<is_unsigned<_StorageType>::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 <class _StorageType, __enable_if_t<is_unsigned<_StorageType>::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 619dc7242a3660..a1797fab6cc140 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 <cstddef>
#include <vector>
+#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<bool, std::uint8_t, std::int8_t>;
+ std::vector<bool, Alloc> in(100, false, Alloc(1));
+ std::vector<bool, Alloc> expected(100, true, Alloc(1));
+ std::fill(in.begin(), in.end(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::fill(in.begin(), in.end(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::fill(in.begin(), in.end(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> 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<char*>(), Test<char>());
types::for_each(types::forward_iterator_list<int*>(), Test<int>());
@@ -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 7d6770de702bf3..582889dbb21d10 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 <algorithm>
#include <cassert>
+#include <vector>
+#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<unsigned> UDI;
template <class Iter>
-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 <class Iter>
-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<char>(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<char>(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<char>(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<char>(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<bool, std::uint8_t, std::int8_t>;
+ std::vector<bool, Alloc> in(100, false, Alloc(1));
+ std::vector<bool, Alloc> expected(100, true, Alloc(1));
+ std::fill_n(in.begin(), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::fill_n(in.begin(), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::fill_n(in.begin(), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::fill_n(in.begin(), in.size(), true);
+ assert(in == expected);
+ }
+}
+
+int main(int, char**) {
+ test_char<cpp17_output_iterator<char*> >();
+ test_char<forward_iterator<char*> >();
+ test_char<bidirectional_iterator<char*> >();
+ test_char<random_access_iterator<char*> >();
+ test_char<char*>();
-int main(int, char**)
-{
- test_char<cpp17_output_iterator<char*> >();
- test_char<forward_iterator<char*> >();
- test_char<bidirectional_iterator<char*> >();
- test_char<random_access_iterator<char*> >();
- test_char<char*>();
+ test_int<cpp17_output_iterator<int*> >();
+ test_int<forward_iterator<int*> >();
+ test_int<bidirectional_iterator<int*> >();
+ test_int<random_access_iterator<int*> >();
+ test_int<int*>();
- test_int<cpp17_output_iterator<int*> >();
- test_int<forward_iterator<int*> >();
- test_int<bidirectional_iterator<int*> >();
- test_int<random_access_iterator<int*> >();
- test_int<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 5dc375e0e8dc0d..870f05bc555143 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 <cassert>
#include <ranges>
#include <string>
+#include <vector>
+#include "sized_allocator.h"
#include "almost_satisfies_types.h"
#include "test_iterators.h"
+#include "test_macros.h"
template <class Iter, class Sent = sentinel_wrapper<Iter>>
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<It> 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<int, 0> 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<bool, Alloc>::iterator` to satisfy the
+// `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, 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<bool, std::uint8_t, std::int8_t>;
+ std::vector<bool, Alloc> in(100, false, Alloc(1));
+ std::vector<bool, Alloc> expected(100, true, Alloc(1));
+ std::ranges::fill(in, true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::ranges::fill(in, true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::ranges::fill(in, true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::ranges::fill(in, true);
+ assert(in == expected);
+ }
+}
+#endif
+
constexpr bool test() {
test_iterators<cpp17_output_iterator<int*>, sentinel_wrapper<cpp17_output_iterator<int*>>>();
test_iterators<cpp20_output_iterator<int*>, sentinel_wrapper<cpp20_output_iterator<int*>>>();
@@ -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<std::ranges::dangling> decltype(auto) ret =
- std::ranges::fill(std::array<int, 10> {}, 1);
+ std::ranges::fill(std::array<int, 10>{}, 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 10ff385d474281..fc3289772b886b 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 <cassert>
#include <ranges>
#include <string>
+#include <vector>
+#include "sized_allocator.h"
#include "almost_satisfies_types.h"
#include "test_iterators.h"
+#include "test_macros.h"
template <class Iter>
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<bool, Alloc>::iterator` to satisfy the
+// `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, 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<bool, std::uint8_t, std::int8_t>;
+ std::vector<bool, Alloc> in(100, false, Alloc(1));
+ std::vector<bool, Alloc> expected(100, true, Alloc(1));
+ std::ranges::fill_n(std::ranges::begin(in), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint16_t, std::int16_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::ranges::fill_n(std::ranges::begin(in), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint32_t, std::int32_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> expected(200, true, Alloc(1));
+ std::ranges::fill_n(std::ranges::begin(in), in.size(), true);
+ assert(in == expected);
+ }
+ {
+ using Alloc = sized_allocator<bool, std::uint64_t, std::int64_t>;
+ std::vector<bool, Alloc> in(200, false, Alloc(1));
+ std::vector<bool, Alloc> 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<cpp17_output_iterator<int*>, sentinel_wrapper<cpp17_output_iterator<int*>>>();
test_iterators<cpp20_output_iterator<int*>, sentinel_wrapper<cpp20_output_iterator<int*>>>();
@@ -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 00000000000000..a877252e82962c
--- /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 <cstddef>
+#include <limits>
+#include <memory>
+#include <new>
+
+#include "test_macros.h"
+
+template <typename T, typename SIZE_TYPE = std::size_t, typename DIFF_TYPE = std::ptrdiff_t>
+class sized_allocator {
+ template <typename U, typename Sz, typename Diff>
+ 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)...
[truncated]
|
ldionne
requested changes
Jan 14, 2025
libcxx/include/__fwd/bit_reference.h
Outdated
| @@ -20,6 +22,22 @@ _LIBCPP_BEGIN_NAMESPACE_STD | |||
| template <class _Cp, bool _IsConst, typename _Cp::__storage_type = 0> | |||
| class __bit_iterator; | |||
|
|
|||
| template <class _StorageType, __enable_if_t<is_unsigned<_StorageType>::value, int> = 0> | |||
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _StorageType __leading_mask(unsigned __shift) { | |||
There was a problem hiding this comment.
I think this function was added but it's not used in this patch, is that right? Perhaps we should only add it in the patch where it gets used?
There was a problem hiding this comment.
You're right. Since I have introduced a new API __fill_range_in_word to set the given bit range of the __bit_reference to a given value following you next comment, the functions that create the bit masks are now incorporated into the new function __fill_range_in_word. Thus, in the PR, I don't need the functions __leading_mask, __trailing_mask, or __middle_mask any more and have been removed.
libcxx/include/__algorithm/fill_n.h
Outdated
Comment on lines
58
to
62
| __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 |
There was a problem hiding this comment.
Would it make sense to instead introduce an API to set the given bits of the __bit_reference to a given value? Dealing with masks in this algorithm seems like pretty low-level, instead perhaps we could move that logic over to __bit_reference itself.
There was a problem hiding this comment.
This makes great sense to me. I have introduced __fill_range_in_word to set given bit range to a specified value. This also leads to removal of __trailing_mask, '__leading_mask', and __middle_mask.
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
9 times, most recently
from
0f1d23a to
5349b7c
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
5 times, most recently
from
b708ad2 to
ac6addf
Compare
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
from
ac6addf to
2e64e4f
Compare
ldionne
approved these changes
Jan 20, 2025
Comment on lines
+19
to
+22
| template <typename T, typename Size = std::size_t, typename Difference = std::ptrdiff_t> | ||
| class sized_allocator { |
There was a problem hiding this comment.
Suggested change
| template <typename T, typename Size = std::size_t, typename Difference = std::ptrdiff_t> | |
| class sized_allocator { | |
| // Allocator with a provided size_type and difference_type, used to test corner | |
| // cases like arithmetic on Allocator::size_type in generic code. | |
| template <typename T, typename Size = std::size_t, typename Difference = std::ptrdiff_t> | |
| class sized_allocator { |
Comment on lines
54
to
55
| // The `ranges::{fill, fill_n}` algorithms require `vector<bool, Alloc>::iterator` to satisfy the | ||
| // `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, which is only met since C++23. |
There was a problem hiding this comment.
Suggested change
| // The `ranges::{fill, fill_n}` algorithms require `vector<bool, Alloc>::iterator` to satisfy the | |
| // `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, which is only met since C++23. | |
| // Make sure we behave properly with std::vector<bool> iterators with custom size types, see | |
| // https://github.com/llvm/llvm-project/pull/122410. | |
| // | |
| // The `ranges::{fill, fill_n}` algorithms require `vector<bool, Alloc>::iterator` to satisfy the | |
| // `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, which is only met since C++23. |
Comment on lines
81
to
82
| // The `ranges::{fill, fill_n}` algorithms require `vector<bool, Alloc>::iterator` to satisfy the | ||
| // `std::indirectly_writable` concept when used with `vector<bool, Alloc>`, which is only met since C++23. |
There was a problem hiding this comment.
Same here, let's add a comment with a link to the PR.
libcxx/include/__bit_reference
Outdated
| template <class _StoragePointer, | ||
| __enable_if_t<is_unsigned<typename pointer_traits<_StoragePointer>::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) { |
There was a problem hiding this comment.
I would switch __ctz and __clz: the leading zeros should probably come before the trailing zeros in the API, since that's the more natural order.
I suggest you apply this change by switching the order of the arguments but not switching the call sites. Then, run the tests and see if something fails. If nothing fails, you just found some missing coverage :-). And if things fail, you can go ahead and fix up the call sites. It shouldn't take you much longer than just making the change, but it'll confirm that we have coverage for this!
There was a problem hiding this comment.
This makes great sense to me. I have switched the function parameters __ctz and __clz in the definition of __fill_masked_range. It seems that I also need to update the call-site arguments to match these two parameters, or I might have misunderstood the comment regarding the call sites. I agree with the comment's intention that this minor change could help us identify potential issues. Therefore, I also adjusted the order of the bitwise operations when calculating the mask within this function.
There was a problem hiding this comment.
If you do find that we're missing coverage and add some tests, I'd like to have another quick look before merging.
Yes, the bitwise optimization algorithms do lack some tests. Current tests for {fill, fill_n, find} are only limited to 1-2 bytes, which is less than one word (generally 8 bytes). Therefore, I've added new tests for {fill, fill_n, find} in #121209 to cover tests for more than 1 word.
Additionally, I've refactored existing tests for other bitwise optimization algorithms to remove redundant code. See #120909 and #121138.
These PRs were submitted prior to the current one. The purpose of these tests is to facilitate subsequent work on optimizing the algorithms for vector<bool>::iterator.
winner245
force-pushed
the
fix-bitwise-logic-in-algo
branch
from
2e64e4f to
787d514
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses an undefined behavior that arises when using the
std::fillandstd::fill_nalgorithms, as well as their ranges counterpartsranges::fillandranges::fill_n, withvector<bool, Alloc>that utilizes a custom-sized allocator with small integral types.When assigning to a
vector<bool, Alloc>via thefillorfill_nalgorithms, if the underlying__storage_type(i.e.,Alloc::size_typeif available) of the word is a small unsigned integral type (e.g.,uint8_t,uint16_t,unsigned short), the internal bitwise logic in__algorithm/fill_n.his subject to integral promotion. The problematic code located in__algorithm/fill_n.his as follows:llvm-project/libcxx/include/__algorithm/fill_n.h
Line 44 in acbd822
Here, the evaluation
~__storage_type(0)is first promoted to~int(0), which evaluates to-1. This leads to a subsequent bitwise left shift of a negative integer value, resulting in UB prior to C++20. Although starting in C++20, bitwise left shift of negative integer values are defined as modulo2^Noperation, we must address this issue for backward compatibility with standards prior to C++20.Since this UB typically results in a compilation warning, it can be difficult to detect. Running the new tests added in this PR without applying my fix will cause only the
stage 2 CI (generic-gcc-cxx11, gcc-14, g++-14)to fail, as it treats theshift-negative-valuewarning as an error using the-Werror=shift-negative-valuecompilation flag (all other stage 1 and 2 CIs pass):To reproduce this UB, we should enable the warning as an error with the following compilation flags in both Clang and GCC:
-Werror=shift-negative-valueand run it for any C++ standards prior to C++20:Godbolt Link
When compiling this code, Clang will produce the following error message (gcc exhibits a similar behavior):
Solution: since integral promotions in bitwise arithmetic for small integral types are inevitable, to resolve this UB, we need to ensure that the result of each bitwise operation is explicitly cast back to the unsigned
__storage_type. This prevents left shifts of negative values due to integral promotions.Additional Note: This UB was first observed while working on #119801 and has led to more serious ambiguous call bugs in
{std, ranges}::{count, find}algorithms, as demonstrated in issue #122528. Thus, fixing the UB forfill/fill_nis a prerequisite for addressing the bugs in the{count, find}algorithms.