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 f7b2481..ddc1405 100644 (file)
--- 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 9678749..6b43b58 100644 (file)
--- 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 (file)
index 0000000..27d37aa
--- /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 (file)
index 0000000..963e153
--- /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 (file)
index 0000000..7eb49b5
--- /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 (file)
index 0000000..e4f2139
--- /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 c308c97..302af9c 100644 (file)
--- 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 (file)
index 78aadc4..0000000
--- 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 }