Bug 114817 - Wrong codegen for std::copy of "trivially copyable but not trivially assignable" type
Summary: Wrong codegen for std::copy of "trivially copyable but not trivially assignab...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: unknown
: P3 normal
Target Milestone: 15.0
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-23 04:02 UTC by Arthur O'Dwyer
Modified: 2024-10-18 14:00 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2024-10-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur O'Dwyer 2024-04-23 04:02:26 UTC
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.
Comment 1 Arthur O'Dwyer 2024-04-23 18:29:53 UTC
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.)
Comment 2 Jiang An 2024-04-24 03:31:04 UTC
Bug 106547 seems somehow related.
Comment 3 Arthur O'Dwyer 2024-04-26 18:43:37 UTC
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&));
Comment 4 Jonathan Wakely 2024-10-11 13:03:20 UTC
For the code in comment 0 we can just disable the memmove optimization when
!__is_trivially_assignable(decltype(*result), decltype(*first))
Comment 5 Jonathan Wakely 2024-10-15 14:45:49 UTC
Patch posted:
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665537.html
Comment 6 GCC Commits 2024-10-18 13:50:59 UTC
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>
Comment 7 Jonathan Wakely 2024-10-18 14:00:14 UTC
Fixed for GCC 15