Jiang An raises an interesting issue over at https://github.com/llvm/llvm-project/pull/89652#discussion_r1575540420 namely: // https://godbolt.org/z/64KGP1avE template<class T, class U> struct EvilPair { T& first; U& second; EvilPair(T& t, U& u) : first(t), second(u) {} EvilPair(const EvilPair&) = default; void operator=(const volatile EvilPair&) = delete; template<class = void> EvilPair& operator=(const EvilPair& rhs) { first = rhs.first; second = rhs.second; return *this; } }; static_assert(std::is_trivially_copyable_v<EvilPair<int&, int&>>); int main() { int a[] = {1,2,3}; int b[] = {4,5,6}; int c[] = {0,0,0}; int d[] = {0,0,0}; EvilPair<int&, int&> ps[] = { {a[0], b[0]}, {a[1], b[1]}, {a[2], b[2]}, }; EvilPair<int&, int&> qs[] = { {c[0], d[0]}, {c[1], d[1]}, {c[2], d[2]}, }; std::copy(ps, ps+3, qs); printf("%d %d %d\n", c[0], c[1], c[2]); } Here, EvilPair is trivially copyable, and also copy-assignable, but it is not trivially copy-assignable. libstdc++'s std::copy assumes that any trivially copyable type can be... well, trivially copied. So it copies the object representation of EvilPair, instead of doing overload resolution to discover that in fact the templated `operator=` should be called instead. Looks like the std::copy optimization was introduced in the GCC 9 release. Allegedly Microsoft STL's `std::pair<int&, int&>` is isomorphic to `EvilPair` these days. libc++'s `std::copy` avoids this issue (AFAICT) by gating their optimization on *both* is_trivially_assignable and is_trivially_copyable. I suspect there will be similar issues with `uninitialized_foo` functions and/or vector reallocation, for types that are trivially copyable (or trivially relocatable!) and yet not trivially destructive-movable.
Yes, vector reallocation has analogous trouble with types that are "trivial, but not trivially copy constructible." https://godbolt.org/z/Psboqf3MP (libc++ happens to sidestep this pitfall *on Clang 15+,* because libc++ gates their vector reallocation optimization differently depending on whether `__has_builtin(__is_trivially_relocatable)`; but libc++ would have the same bug as libstdc++ when compiled on GCC.)
Bug 106547 seems somehow related.
https://github.com/boostorg/container/issues/153 , from 2020, is another similar issue. There, boost::container::vector had assumed that if __has_trivial_copy(T) and is_copy_constructible_v<T>, then T could be relocated via memcpy; but in fact __has_trivial_copy(T) is also true for move-only types. So it was possible to create a type such that (__has_trivial_copy(T) && __is_constructible(T, T&)) and yet not __is_trivially_constructible(T, T&). // https://godbolt.org/z/x5Wda1M6E struct T { template<class U> T(U&&); T(T&&); ~T(); }; static_assert(__has_trivial_copy(T) && __is_constructible(T, T&)); // ...and yet... static_assert(not __is_trivially_constructible(T, T&));
For the code in comment 0 we can just disable the memmove optimization when !__is_trivially_assignable(decltype(*result), decltype(*first))
Patch posted: https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665537.html
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:7ed561f63e7955df4d194669998176df5ef47803 commit r15-4475-g7ed561f63e7955df4d194669998176df5ef47803 Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu Jun 27 13:01:18 2024 +0100 libstdc++: Inline memmove optimizations for std::copy etc. [PR115444] This removes all the __copy_move class template specializations that decide how to optimize std::copy and std::copy_n. We can inline those optimizations into the algorithms, using if-constexpr (and macros for C++98 compatibility) and remove the code dispatching to the various class template specializations. Doing this means we implement the optimization directly for std::copy_n instead of deferring to std::copy, That avoids the unwanted consequence of advancing the iterator in copy_n only to take the difference later to get back to the length that we already had in copy_n originally (as described in PR 115444). With the new flattened implementations, we can also lower contiguous iterators to pointers in std::copy/std::copy_n/std::copy_backwards, so that they benefit from the same memmove optimizations as pointers. There's a subtlety though: contiguous iterators can potentially throw exceptions to exit the algorithm early. So we can only transform the loop to memmove if dereferencing the iterator is noexcept. We don't check that incrementing the iterator is noexcept because we advance the contiguous iterators before using memmove, so that if incrementing would throw, that happens first. I am writing a proposal (P3349R0) which would make this unnecessary, so I hope we can drop the nothrow requirements later. This change also solves PR 114817 by checking is_trivially_assignable before optimizing copy/copy_n etc. to memmove. It's not enough to check that the types are trivially copyable (a precondition for using memmove at all), we also need to check that the specific assignment that would be performed by the algorithm is also trivial. Replacing a non-trivial assignment with memmove would be observable, so not allowed. libstdc++-v3/ChangeLog: PR libstdc++/115444 PR libstdc++/114817 * include/bits/stl_algo.h (__copy_n): Remove generic overload and overload for random access iterators. (copy_n): Inline generic version of __copy_n here. Do not defer to std::copy for random access iterators. * include/bits/stl_algobase.h (__copy_move): Remove. (__nothrow_contiguous_iterator, __memcpyable_iterators): New concepts. (__assign_one, _GLIBCXX_TO_ADDR, _GLIBCXX_ADVANCE): New helpers. (__copy_move_a2): Inline __copy_move logic and conditional memmove optimization into the most generic overload. (__copy_n_a): Likewise. (__copy_move_backward): Remove. (__copy_move_backward_a2): Inline __copy_move_backward logic and memmove optimization into the most generic overload. * testsuite/20_util/specialized_algorithms/uninitialized_copy/114817.cc: New test. * testsuite/20_util/specialized_algorithms/uninitialized_copy_n/114817.cc: New test. * testsuite/25_algorithms/copy/114817.cc: New test. * testsuite/25_algorithms/copy/115444.cc: New test. * testsuite/25_algorithms/copy_n/114817.cc: New test. Reviewed-by: Patrick Palka <ppalka@redhat.com>
Fixed for GCC 15