[committed] libstdc++: Fix conditions for optimizing uninitialized algos [PR102064]
Jonathan Wakely
jwakely@redhat.com
Wed Aug 25 21:29:15 GMT 2021
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 <jwakely@redhat.com>
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.
Tested powerpc64le-linux. Committed to trunk.
-------------- next part --------------
commit ead408529d7a69873a7c14dd12fa043cd5862253
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Aug 25 21:10:48 2021
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 <jwakely@redhat.com>
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.
diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
index f7b24818fc4..ddc1405cb0e 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<typename _ValueType, typename _Tp>
+ 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<T, U>()
+#else
+// No need to check if is_constructible<T, U> 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<bool _TrivialValueTypes>
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_integral<_Size>, 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 <typename _ForwardIterator>
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 <typename _ForwardIterator, typename _Size>
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 <typename _ForwardIterator>
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 <typename _ForwardIterator, typename _Size>
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 <typename _InputIterator, typename _ForwardIterator>
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 <typename _InputIterator, typename _Size, typename _ForwardIterator>
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 9678749befa..6b43b58b277 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 00000000000..27d37aab09f
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_copy/102064.cc
@@ -0,0 +1,52 @@
+// { dg-do compile }
+
+#include <memory>
+#include <algorithm>
+
+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<X>::value, "" );
+#endif
+
+void test01_pr102064()
+{
+ unsigned char buf[sizeof(X)];
+ X* addr = reinterpret_cast<X*>(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<Z>::value, "" );
+
+void test02_pr102064()
+{
+ unsigned char buf[sizeof(Z)];
+ Z* addr = reinterpret_cast<Z*>(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 00000000000..963e1531a71
--- /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 <memory>
+#include <algorithm>
+
+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<X>::value, "" );
+
+void test01_pr102064()
+{
+ unsigned char buf[sizeof(X)];
+ X* addr = reinterpret_cast<X*>(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<Z>::value, "" );
+
+void test02_pr102064()
+{
+ unsigned char buf[sizeof(Z)];
+ Z* addr = reinterpret_cast<Z*>(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 00000000000..7eb49b543a4
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill/102064.cc
@@ -0,0 +1,51 @@
+// { dg-do compile }
+
+#include <memory>
+#include <algorithm>
+
+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<X>::value, "" );
+#endif
+
+void test01_pr102064()
+{
+ unsigned char buf[sizeof(X)];
+ X* addr = reinterpret_cast<X*>(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<Z>::value, "" );
+
+void test02_pr102064()
+{
+ unsigned char buf[sizeof(Z)];
+ Z* addr = reinterpret_cast<Z*>(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 00000000000..e4f2139cf6d
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/specialized_algorithms/uninitialized_fill_n/102064.cc
@@ -0,0 +1,51 @@
+// { dg-do compile }
+
+#include <memory>
+#include <algorithm>
+
+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<X>::value, "" );
+#endif
+
+void test01_pr102064()
+{
+ unsigned char buf[sizeof(X)];
+ X* addr = reinterpret_cast<X*>(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<Z>::value, "" );
+
+void test02_pr102064()
+{
+ unsigned char buf[sizeof(Z)];
+ Z* addr = reinterpret_cast<Z*>(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 c308c9755e2..302af9c2e97 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<X> 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<Y> with C++17 guaranteed copy elision,
+ // but shouldn't be able to copy from it with uninitialized_copy:
+ std::vector<Y> 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 78aadc4798b..00000000000
--- 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
-// <http://www.gnu.org/licenses/>.
-
-// { dg-do compile { target c++17 } }
-
-#include <vector>
-
-// 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<X> v1{x, x+1}; // { dg-error "here" }
-
- // Should not be able to create vector using uninitialized_fill_n:
- std::vector<X> v2{2u, X{}}; // { dg-error "here" }
-}
-
-void test02()
-{
-#if __cplusplus >= 201703L
- // Can create initializer_list<X> with C++17 guaranteed copy elision,
- // but shouldn't be able to copy from it with uninitialized_copy:
- std::vector<X> v3{X{}, X{}, X{}}; // { dg-error "here" }
-#endif
-}
-// { dg-error "constructible from value" "" { target *-*-* } 0 }
-// { dg-error "constructible from input" "" { target *-*-* } 0 }
More information about the Libstdc++
mailing list