[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #13 from mfarca --- Would you please backport this to 12 when the patch lands?
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #12 from mfarca --- I've applied the above patch directly to `/usr/include/c++/12/bits/stl_algobase.h` under my fedora 36 env with gcc 12.2.1 and it solved the problem.
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960
--- Comment #11 from Jonathan Wakely ---
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -1824,11 +1824,14 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
}
#if __cpp_lib_three_way_comparison
- // Iter points to a contiguous range of unsigned narrow character type
- // or std::byte, suitable for comparison by memcmp.
- template
-concept __is_byte_iter = contiguous_iterator<_Iter>
- && __is_memcmp_ordered>::__value;
+ // Both iterators refer to contiguous ranges of unsigned narrow characters,
+ // or std::byte, or big-endian unsigned integers, suitable for comparison
+ // using memcmp.
+ template
+concept __memcmp_ordered_with
+ = (__is_memcmp_ordered_with,
+ iter_value_t<_Iter2>>::__value)
+ && contiguous_iterator<_Iter1> && contiguous_iterator<_Iter2>;
// Return a struct with two members, initialized to the smaller of x and y
// (or x if they compare equal) and the result of the comparison x <=> y.
@@ -1878,20 +1881,20 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
if (!std::__is_constant_evaluated())
if constexpr (same_as<_Comp, __detail::_Synth3way>
|| same_as<_Comp, compare_three_way>)
- if constexpr (__is_byte_iter<_InputIter1>)
- if constexpr (__is_byte_iter<_InputIter2>)
- {
- const auto [__len, __lencmp] = _GLIBCXX_STD_A::
- __min_cmp(__last1 - __first1, __last2 - __first2);
- if (__len)
- {
- const auto __c
- = __builtin_memcmp(&*__first1, &*__first2, __len) <=> 0;
- if (__c != 0)
- return __c;
- }
- return __lencmp;
- }
+ if constexpr (__memcmp_ordered_with<_InputIter1, _InputIter2>)
+ {
+ const auto [__len, __lencmp] = _GLIBCXX_STD_A::
+ __min_cmp(__last1 - __first1, __last2 - __first2);
+ if (__len)
+ {
+ const auto __blen = __len * sizeof(*__first1);
+ const auto __c
+ = __builtin_memcmp(&*__first1, &*__first2, __blen) <=> 0;
+ if (__c != 0)
+ return __c;
+ }
+ return __lencmp;
+ }
while (__first1 != __last1)
{
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #10 from Jonathan Wakely --- Oh I already defined a __is_memcmp_ordered_with trait, which does the same-size check. I think that's what should be used here.
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #9 from Stefan Schulze Frielinghaus --- (In reply to Jonathan Wakely from comment #7) > We can't use memcmp if the sizes are different. We don't want to use the > min, we want to guard that code with the sizes being the same, then we can > just use len*sizeof(*first1) because we know it's the same as > sizeof(*first2). Hehe I was about to add another comment. I just confused myself with taking the minimum but we rather need to ensure that we are walking over same sized integers. LGTM
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960
--- Comment #8 from Jonathan Wakely ---
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -1824,8 +1824,9 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
}
#if __cpp_lib_three_way_comparison
- // Iter points to a contiguous range of unsigned narrow character type
- // or std::byte, suitable for comparison by memcmp.
+ // Iter points to a contiguous range of unsigned narrow character type,
+ // or std::byte, or big-endian unsigned integers, suitable for comparison
+ // by memcmp.
template
concept __is_byte_iter = contiguous_iterator<_Iter>
&& __is_memcmp_ordered>::__value;
@@ -1879,14 +1880,16 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
if constexpr (same_as<_Comp, __detail::_Synth3way>
|| same_as<_Comp, compare_three_way>)
if constexpr (__is_byte_iter<_InputIter1>)
- if constexpr (__is_byte_iter<_InputIter2>)
+ if constexpr (__is_byte_iter<_InputIter2>
+ && sizeof(*__first1) == sizeof(*__first2))
{
const auto [__len, __lencmp] = _GLIBCXX_STD_A::
__min_cmp(__last1 - __first1, __last2 - __first2);
if (__len)
{
+ const auto __blen = __len * sizeof(*__first1);
const auto __c
- = __builtin_memcmp(&*__first1, &*__first2, __len) <=> 0;
+ = __builtin_memcmp(&*__first1, &*__first2, __blen) <=> 0;
if (__c != 0)
return __c;
}
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #7 from Jonathan Wakely --- Ohhh, I forgot I did that, sorry! Yeah, the memcmp code wasn't updated to match the different behaviour of __is_byte_iter for BE. We can't use memcmp if the sizes are different. We don't want to use the min, we want to guard that code with the sizes being the same, then we can just use len*sizeof(*first1) because we know it's the same as sizeof(*first2).
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #6 from Stefan Schulze Frielinghaus --- Guard __is_byte_iter checks for contiguous bytes which I guess is fine for std::vector and then checks for __is_memcmp_ordered which is fine for big-endian targets in conjunction with unsigned integers. From cpp_type_traits.h we have: // Whether memcmp can be used to determine ordering for a type // e.g. in std::lexicographical_compare or three-way comparisons. // True for unsigned integer-like types where comparing each byte in turn // as an unsigned char yields the right result. This is true for all // unsigned integers on big endian targets, but only unsigned narrow // character types (and std::byte) on little endian targets. template::__value #else __is_byte<_Tp>::__value #endif Thus using memcmp here is fine, however, I'm still a bit unsure whether we really have to take the minimum of *__first1 and *__first2 since I haven't found any size-relation between those types.
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 --- Comment #5 from Jonathan Wakely --- But it's guarded by: if constexpr (__is_byte_iter<_InputIter1>) if constexpr (__is_byte_iter<_InputIter2>) This condition is only supposed to be true when sizeof(*__first1) == 1 and sizeof(*__first2) == 1 We can only use memcmp if we're comparing single bytes as unsigned values (and if the iterators are pointers to contiguous memory, not e.g. segmented iterators like std::deque's, or not even random access iterators, like std::list's). For std::vector we should not use this code at all.
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 Stefan Schulze Frielinghaus changed: What|Removed |Added CC||jwakely at redhat dot com --- Comment #4 from Stefan Schulze Frielinghaus --- While giving it a second thought maybe something like const auto __len_bytes = __len * std::min (sizeof (*__first1), sizeof (*__first2)); would be more appropriate since AFAICT the types _InputIter1 and _InputIter2 are not related to each other w.r.t. to their pointed size. Maybe Jonathan can shed some light on this?
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960
--- Comment #3 from Stefan Schulze Frielinghaus
---
This seems to be a bug in the three way comparison introduced with C++20. The
bug happens while deciding whether key v2 already exists in the map or not.
template
constexpr auto
lexicographical_compare_three_way(_InputIter1 __first1,
_InputIter1 __last1,
_InputIter2 __first2,
_InputIter2 __last2,
_Comp __comp)
-> decltype(__comp(*__first1, *__first2))
{
// concept requirements
__glibcxx_function_requires(_InputIteratorConcept<_InputIter1>)
__glibcxx_function_requires(_InputIteratorConcept<_InputIter2>)
__glibcxx_requires_valid_range(__first1, __last1);
__glibcxx_requires_valid_range(__first2, __last2);
using _Cat = decltype(__comp(*__first1, *__first2));
static_assert(same_as, _Cat>);
if (!std::__is_constant_evaluated())
if constexpr (same_as<_Comp, __detail::_Synth3way>
|| same_as<_Comp, compare_three_way>)
if constexpr (__is_byte_iter<_InputIter1>)
if constexpr (__is_byte_iter<_InputIter2>)
{
const auto [__len, __lencmp] = _GLIBCXX_STD_A::
__min_cmp(__last1 - __first1, __last2 - __first2);
if (__len)
{
const auto __c
= __builtin_memcmp(&*__first1, &*__first2, __len) <=> 0;
if (__c != 0)
return __c;
}
return __lencmp;
}
__len equals 1 since both vectors have length 1. However, memcmp should be
called with the number of bytes and not the number of elements of the vector.
That means memcmp is called with two pointers to MEMs of unsigned shorts 1 and
2 where the high-bytes equal 0 and therefore memcmp returns with 0 on
big-endian targets. Ultimately __lencmp is returned which itself equals
std::strong_ordering::equal rendering v2 replacing v1.
Fixed by
diff --git a/libstdc++-v3/include/bits/stl_algobase.h
b/libstdc++-v3/include/bits/stl_algobase.h
index d534e02871f..6ebece315f7 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -1867,8 +1867,10 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
__min_cmp(__last1 - __first1, __last2 - __first2);
if (__len)
{
+ const auto __len_bytes = __len * sizeof (*first1);
const auto __c
- = __builtin_memcmp(&*__first1, &*__first2, __len) <=> 0;
+ = __builtin_memcmp(&*__first1, &*__first2, __len_bytes)
+ <=> 0;
if (__c != 0)
return __c;
}
Can you give the patch a try?
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 Richard Biener changed: What|Removed |Added Ever confirmed|0 |1 Status|UNCONFIRMED |NEW Last reconfirmed||2024-02-19 Known to fail||12.3.0, 13.2.1 --- Comment #2 from Richard Biener --- works fine on x86_64-linux. Confirmed on s390x-linux with g++-13 (SUSE Linux) 13.2.1 20230912 [revision b96e66fd4ef3e36983969fb8cdd1956f551a074b] and g++-12 (SUSE Linux) 12.3.0
[Bug target/113960] std::map with std::vector as input overwrites itself with c++20, on s390x platform
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 Sam James changed: What|Removed |Added CC||sjames at gcc dot gnu.org --- Comment #1 from Sam James --- I can reproduce it w/ 13.2.1 20231216 although I'd prefer to leave it to someone else to confirm...
