From ead408529d7a69873a7c14dd12fa043cd5862253 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Wed, 25 Aug 2021 21:10:48 +0100 Subject: [PATCH] libstdc++: Fix conditions for optimizing uninitialized algos [PR102064] While laying some groundwork for constexpr std::vector, I noticed some bugs in the std::uninitialized_xxx algorithms. The conditions being checked for optimizing trivial cases were not quite right, as shown in the examples in the PR. This consolidates the checks into a single macro. The macro has appropriate definitions for C++98 or for later standards, to avoid a #if everywhere the checks are used. For C++11 and later the check makes a call to a new function doing a static_assert to ensure we don't use assignment in cases where construction would have been invalid. Extracting that check to a separate function will be useful for constexpr std::vector, as that can't use std::uninitialized_copy directly because it isn't constexpr). The consolidated checks mean that some slight variations in static assert message are gone, as there is only one place that does the assert now. That required adjusting some tests. As part of that the redundant 89164_c++17.cc test was merged into 89164.cc which is compiled as C++17 by default now, but can also use other -std options if the C++17-specific error is made conditional with a target selector. Signed-off-by: Jonathan Wakely libstdc++-v3/ChangeLog: PR libstdc++/102064 * include/bits/stl_uninitialized.h (_GLIBCXX_USE_ASSIGN_FOR_INIT): Define macro to check conditions for optimizing trivial cases. (__check_constructible): New function to do static assert. (uninitialized_copy, uninitialized_fill, uninitialized_fill_n): Use new macro. * testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc: Adjust dg-error pattern. * testsuite/23_containers/vector/cons/89164.cc: Likewise. Add C++17-specific checks from 89164_c++17.cc. * testsuite/23_containers/vector/cons/89164_c++17.cc: Removed. * testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc: New test. * testsuite/20_util/specialized_algorithms/uninitialized_copy_n/102064.cc: New test. * testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc: New test. * testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc: New test. --- libstdc++-v3/include/bits/stl_uninitialized.h | 100 +++++++++++------- .../uninitialized_copy/1.cc | 2 +- .../uninitialized_copy/102064.cc | 52 +++++++++ .../uninitialized_copy_n/102064.cc | 48 +++++++++ .../uninitialized_fill/102064.cc | 51 +++++++++ .../uninitialized_fill_n/102064.cc | 51 +++++++++ .../23_containers/vector/cons/89164.cc | 14 ++- .../23_containers/vector/cons/89164_c++17.cc | 49 --------- 8 files changed, 275 insertions(+), 92 deletions(-) create mode 100644 libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc create mode 100644 libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy_n/102064.cc create mode 100644 libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc create mode 100644 libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc delete mode 100644 libstdc++-v3/testsuite/23_containers/vector/cons/89164_c++17.cc diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h index f7b24818fc45..ddc1405cb0e6 100644 --- a/libstdc++-v3/include/bits/stl_uninitialized.h +++ b/libstdc++-v3/include/bits/stl_uninitialized.h @@ -77,6 +77,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION /// @cond undocumented +#if __cplusplus >= 201103L + template + constexpr bool + __check_constructible() + { + // Trivial types can have deleted constructors, but std::copy etc. + // only use assignment (or memmove) not construction, so we need an + // explicit check that construction from _Tp is actually valid, + // otherwise some ill-formed uses of std::uninitialized_xxx would + // compile without errors. This gives a nice clear error message. + static_assert(is_constructible<_ValueType, _Tp>::value, + "result type must be constructible from input type"); + + return true; + } + +// If the type is trivial we don't need to construct it, just assign to it. +// But trivial types can still have deleted or inaccessible assignment, +// so don't try to use std::copy or std::fill etc. if we can't assign. +# define _GLIBCXX_USE_ASSIGN_FOR_INIT(T, U) \ + __is_trivial(T) && __is_assignable(T&, U) \ + && std::__check_constructible() +#else +// No need to check if is_constructible for C++98. Trivial types have +// no user-declared constructors, so if the assignment is valid, construction +// should be too. +# define _GLIBCXX_USE_ASSIGN_FOR_INIT(T, U) \ + __is_trivial(T) && __is_assignable(T&, U) +#endif + template struct __uninitialized_copy { @@ -130,24 +160,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _ValueType1; typedef typename iterator_traits<_ForwardIterator>::value_type _ValueType2; + + // _ValueType1 must be trivially-copyable to use memmove, so don't + // both optimizing to std::copy if it isn't. + // XXX Unnecessary because std::copy would check it anyway? + const bool __can_memmove = __is_trivial(_ValueType1); + #if __cplusplus < 201103L - const bool __assignable = true; + typedef typename iterator_traits<_InputIterator>::reference _From; #else - // Trivial types can have deleted copy constructor, but the std::copy - // optimization that uses memmove would happily "copy" them anyway. - static_assert(is_constructible<_ValueType2, decltype(*__first)>::value, - "result type must be constructible from value type of input range"); - - typedef typename iterator_traits<_InputIterator>::reference _RefType1; - typedef typename iterator_traits<_ForwardIterator>::reference _RefType2; - // Trivial types can have deleted assignment, so using std::copy - // would be ill-formed. Require assignability before using std::copy: - const bool __assignable = is_assignable<_RefType2, _RefType1>::value; + using _From = decltype(*__first); #endif + const bool __assignable + = _GLIBCXX_USE_ASSIGN_FOR_INIT(_ValueType2, _From); - return std::__uninitialized_copy<__is_trivial(_ValueType1) - && __is_trivial(_ValueType2) - && __assignable>:: + return std::__uninitialized_copy<__can_memmove && __assignable>:: __uninit_copy(__first, __last, __result); } @@ -203,20 +230,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { typedef typename iterator_traits<_ForwardIterator>::value_type _ValueType; -#if __cplusplus < 201103L - const bool __assignable = true; -#else - // Trivial types can have deleted copy constructor, but the std::fill - // optimization that uses memmove would happily "copy" them anyway. - static_assert(is_constructible<_ValueType, const _Tp&>::value, - "result type must be constructible from input type"); - // Trivial types can have deleted assignment, so using std::fill - // would be ill-formed. Require assignability before using std::fill: - const bool __assignable = is_copy_assignable<_ValueType>::value; -#endif + // Trivial types do not need a constructor to begin their lifetime, + // so try to use std::fill to benefit from its memset optimization. + const bool __can_fill + = _GLIBCXX_USE_ASSIGN_FOR_INIT(_ValueType, const _Tp&); - std::__uninitialized_fill<__is_trivial(_ValueType) && __assignable>:: + std::__uninitialized_fill<__can_fill>:: __uninit_fill(__first, __last, __x); } @@ -276,27 +296,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _ValueType; // Trivial types do not need a constructor to begin their lifetime, - // so try to use std::fill_n to benefit from its memmove optimization. + // so try to use std::fill_n to benefit from its optimizations. + const bool __can_fill + = _GLIBCXX_USE_ASSIGN_FOR_INIT(_ValueType, const _Tp&) // For arbitrary class types and floating point types we can't assume // that __n > 0 and std::__size_to_integer(__n) > 0 are equivalent, // so only use std::fill_n when _Size is already an integral type. -#if __cplusplus < 201103L - const bool __can_fill = __is_integer<_Size>::__value; -#else - // Trivial types can have deleted copy constructor, but the std::fill_n - // optimization that uses memmove would happily "copy" them anyway. - static_assert(is_constructible<_ValueType, const _Tp&>::value, - "result type must be constructible from input type"); + && __is_integer<_Size>::__value; - // Trivial types can have deleted assignment, so using std::fill_n - // would be ill-formed. Require assignability before using std::fill_n: - constexpr bool __can_fill - = __and_, is_copy_assignable<_ValueType>>::value; -#endif - return __uninitialized_fill_n<__is_trivial(_ValueType) && __can_fill>:: + return __uninitialized_fill_n<__can_fill>:: __uninit_fill_n(__first, __n, __x); } +#undef _GLIBCXX_USE_ASSIGN_FOR_INIT + /// @cond undocumented // Extensions: versions of uninitialized_copy, uninitialized_fill, @@ -864,6 +877,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __n The number of elements to copy. * @param __result An output iterator. * @return __result + __n + * @since C++11 * * Like copy_n(), but does not require an initialized output range. */ @@ -894,6 +908,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @brief Default-initializes objects in the range [first,last). * @param __first A forward iterator. * @param __last A forward iterator. + * @since C++17 */ template inline void @@ -908,6 +923,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __first A forward iterator. * @param __count The number of objects to construct. * @return __first + __count + * @since C++17 */ template inline _ForwardIterator @@ -920,6 +936,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @brief Value-initializes objects in the range [first,last). * @param __first A forward iterator. * @param __last A forward iterator. + * @since C++17 */ template inline void @@ -934,6 +951,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __first A forward iterator. * @param __count The number of objects to construct. * @return __result + __count + * @since C++17 */ template inline _ForwardIterator @@ -948,6 +966,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __last An input iterator. * @param __result An output iterator. * @return __result + (__first - __last) + * @since C++17 */ template inline _ForwardIterator @@ -965,6 +984,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION * @param __count The number of objects to initialize. * @param __result An output iterator. * @return __result + __count + * @since C++17 */ template inline pair<_InputIterator, _ForwardIterator> diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc index 9678749befaf..6b43b58b2776 100644 --- a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/1.cc @@ -34,4 +34,4 @@ test01(T* result) T t[1]; std::uninitialized_copy(t, t+1, result); // { dg-error "here" } } -// { dg-error "constructible from value" "" { target *-*-* } 0 } +// { dg-error "must be constructible from input type" "" { target *-*-* } 0 } diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc new file mode 100644 index 000000000000..27d37aab09f4 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc @@ -0,0 +1,52 @@ +// { dg-do compile } + +#include +#include + +struct X; + +struct Y +{ + operator X() const; +}; + +struct X +{ +private: + void operator=(const Y&); +}; + +Y::operator X() const { return X(); } + +#if __cplusplus >= 201103L +static_assert( std::is_trivial::value, "" ); +#endif + +void test01_pr102064() +{ + unsigned char buf[sizeof(X)]; + X* addr = reinterpret_cast(buf); + const Y y[1] = { }; + std::uninitialized_copy(y, y + 1, addr); +} + +#if __cplusplus >= 201103L +struct Z +{ + Z() = default; + Z(int) { } + Z(const Z&) = default; + Z& operator=(const Z&) = default; + Z& operator=(int) = delete; +}; + +static_assert( std::is_trivial::value, "" ); + +void test02_pr102064() +{ + unsigned char buf[sizeof(Z)]; + Z* addr = reinterpret_cast(buf); + const int i[1] = { 99 }; + std::uninitialized_copy(i, i + 1, addr); +} +#endif diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy_n/102064.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy_n/102064.cc new file mode 100644 index 000000000000..963e1531a71b --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy_n/102064.cc @@ -0,0 +1,48 @@ +// { dg-do compile { target c++11 } } + +#include +#include + +struct X; + +struct Y +{ + operator X() const; +}; + +struct X +{ +private: + void operator=(const Y&); +}; + +Y::operator X() const { return X(); } + +static_assert( std::is_trivial::value, "" ); + +void test01_pr102064() +{ + unsigned char buf[sizeof(X)]; + X* addr = reinterpret_cast(buf); + const Y y[1] = { }; + std::uninitialized_copy_n(y, 1, addr); +} + +struct Z +{ + Z() = default; + Z(int) { } + Z(const Z&) = default; + Z& operator=(const Z&) = default; + Z& operator=(int) = delete; +}; + +static_assert( std::is_trivial::value, "" ); + +void test02_pr102064() +{ + unsigned char buf[sizeof(Z)]; + Z* addr = reinterpret_cast(buf); + const int i[1] = { 99 }; + std::uninitialized_copy_n(i, 1, addr); +} diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc new file mode 100644 index 000000000000..7eb49b543a41 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc @@ -0,0 +1,51 @@ +// { dg-do compile } + +#include +#include + +struct X; + +struct Y +{ + operator X() const; +}; + +struct X +{ +private: + void operator=(const Y&); +}; + +Y::operator X() const { return X(); } + +#if __cplusplus >= 201103L +static_assert( std::is_trivial::value, "" ); +#endif + +void test01_pr102064() +{ + unsigned char buf[sizeof(X)]; + X* addr = reinterpret_cast(buf); + Y y; + std::uninitialized_fill(addr, addr + 1, y); +} + +#if __cplusplus >= 201103L +struct Z +{ + Z() = default; + Z(int) { } + Z(const Z&) = default; + Z& operator=(const Z&) = default; + Z& operator=(int) = delete; +}; + +static_assert( std::is_trivial::value, "" ); + +void test02_pr102064() +{ + unsigned char buf[sizeof(Z)]; + Z* addr = reinterpret_cast(buf); + std::uninitialized_fill(addr, addr + 1, 99); +} +#endif diff --git a/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc new file mode 100644 index 000000000000..e4f2139cf6da --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc @@ -0,0 +1,51 @@ +// { dg-do compile } + +#include +#include + +struct X; + +struct Y +{ + operator X() const; +}; + +struct X +{ +private: + void operator=(const Y&); +}; + +Y::operator X() const { return X(); } + +#if __cplusplus >= 201103L +static_assert( std::is_trivial::value, "" ); +#endif + +void test01_pr102064() +{ + unsigned char buf[sizeof(X)]; + X* addr = reinterpret_cast(buf); + Y y; + std::uninitialized_fill_n(addr, 1, y); +} + +#if __cplusplus >= 201103L +struct Z +{ + Z() = default; + Z(int) { } + Z(const Z&) = default; + Z& operator=(const Z&) = default; + Z& operator=(int) = delete; +}; + +static_assert( std::is_trivial::value, "" ); + +void test02_pr102064() +{ + unsigned char buf[sizeof(Z)]; + Z* addr = reinterpret_cast(buf); + std::uninitialized_fill_n(addr, 1, 99); +} +#endif diff --git a/libstdc++-v3/testsuite/23_containers/vector/cons/89164.cc b/libstdc++-v3/testsuite/23_containers/vector/cons/89164.cc index c308c9755e28..302af9c2e97f 100644 --- a/libstdc++-v3/testsuite/23_containers/vector/cons/89164.cc +++ b/libstdc++-v3/testsuite/23_containers/vector/cons/89164.cc @@ -36,5 +36,15 @@ void test01() // Should not be able to create vector using uninitialized_fill_n: std::vector v2{2u, X{}}; // { dg-error "here" } } -// { dg-error "constructible from value" "" { target *-*-* } 0 } -// { dg-error "constructible from input" "" { target *-*-* } 0 } + +void test02() +{ +#if __cplusplus >= 201703L + struct Y : X { }; + // Can create initializer_list with C++17 guaranteed copy elision, + // but shouldn't be able to copy from it with uninitialized_copy: + std::vector v3{Y{}, Y{}, Y{}}; // { dg-error "here" "" { target c++17 } } +#endif +} + +// { dg-error "must be constructible from input type" "" { target *-*-* } 0 } diff --git a/libstdc++-v3/testsuite/23_containers/vector/cons/89164_c++17.cc b/libstdc++-v3/testsuite/23_containers/vector/cons/89164_c++17.cc deleted file mode 100644 index 78aadc4798b3..000000000000 --- a/libstdc++-v3/testsuite/23_containers/vector/cons/89164_c++17.cc +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright (C) 2019-2021 Free Software Foundation, Inc. -// -// This file is part of the GNU ISO C++ Library. This library is free -// software; you can redistribute it and/or modify it under the -// terms of the GNU General Public License as published by the -// Free Software Foundation; either version 3, or (at your option) -// any later version. - -// This library is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License along -// with this library; see the file COPYING3. If not see -// . - -// { dg-do compile { target c++17 } } - -#include - -// PR libstdc++/89164 - -struct X -{ - X() = default; - X(const X&) = delete; -}; - -void test01() -{ - X x[1]; - // Should not be able to create vector using uninitialized_copy: - std::vector v1{x, x+1}; // { dg-error "here" } - - // Should not be able to create vector using uninitialized_fill_n: - std::vector v2{2u, X{}}; // { dg-error "here" } -} - -void test02() -{ -#if __cplusplus >= 201703L - // Can create initializer_list with C++17 guaranteed copy elision, - // but shouldn't be able to copy from it with uninitialized_copy: - std::vector v3{X{}, X{}, X{}}; // { dg-error "here" } -#endif -} -// { dg-error "constructible from value" "" { target *-*-* } 0 } -// { dg-error "constructible from input" "" { target *-*-* } 0 }