libstdc++: Fix std::basic_stacktrace special members [PR105031]
authorJonathan Wakely <jwakely@redhat.com>
Mon, 11 Apr 2022 16:44:14 +0000 (17:44 +0100)
committerJonathan Wakely <jwakely@redhat.com>
Mon, 11 Apr 2022 16:59:50 +0000 (17:59 +0100)
The PR points out that there is a non-constant condition used for an
if-constexpr statement, but there are several other problems with the
copy, move and swap members of std::basic_stacktrace.

libstdc++-v3/ChangeLog:

PR libstdc++/105031
* include/std/stacktrace (basic_stacktrace::basic_stacktrace):
Fix allocator usage in constructors.
(basic_stacktrace::operator=(const basic_stacktrace&)): Do not
try to reallocate using const allocator.
(basic_stacktrace::operator=(basic_stacktrace&&)): Fix
if-constexpr with non-constant condition. Do not allocate new
storage if allocator propagates. Do not set _M_size if
allocation fails.
(basic_stacktrace::swap(basic_stacktrace&)): Fix typo. Add
assertion that non-propagating allocators are equal.
* testsuite/19_diagnostics/stacktrace/stacktrace.cc: New test.

libstdc++-v3/include/std/stacktrace
libstdc++-v3/testsuite/19_diagnostics/stacktrace/stacktrace.cc [new file with mode: 0644]

index 4e271ce..dd78c71 100644 (file)
@@ -301,7 +301,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { }
 
       basic_stacktrace(const basic_stacktrace& __other) noexcept
-      : basic_stacktrace(__other, __other._M_alloc)
+      : basic_stacktrace(__other,
+         _AllocTraits::select_on_container_copy_construction(__other._M_alloc))
       { }
 
       basic_stacktrace(basic_stacktrace&& __other) noexcept
@@ -326,13 +327,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_alloc(__alloc)
       {
        if constexpr (_Allocator::is_always_equal::value)
-         {
-           _M_impl = std::__exchange(__other._M_impl, {});
-         }
+         _M_impl = std::__exchange(__other._M_impl, {});
        else if (_M_alloc == __other._M_alloc)
-         {
-           _M_impl = std::__exchange(__other._M_impl, {});
-         }
+         _M_impl = std::__exchange(__other._M_impl, {});
+       else if (const auto __s = __other._M_impl._M_size)
+         if (auto __f = _M_impl._M_allocate(_M_alloc, __s))
+           {
+             std::uninitialized_copy_n(__other.begin(), __s, __f);
+             _M_impl._M_size = __s;
+           }
       }
 
       basic_stacktrace&
@@ -361,9 +364,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            // Need to allocate new storage.
            _M_clear();
 
-           // Use the allocator we will have after this function returns.
-           auto& __alloc = __pocca ? __other._M_alloc : _M_alloc;
-           if (auto __f = _M_impl._M_allocate(__alloc, __s))
+           if constexpr (__pocca)
+             _M_alloc = __other._M_alloc;
+
+           if (auto __f = _M_impl._M_allocate(_M_alloc, __s))
              {
                std::uninitialized_copy_n(__other.begin(), __s, __f);
                _M_impl._M_size = __s;
@@ -376,10 +380,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            auto __to = std::copy_n(__other.begin(), __s, begin());
            std::destroy(__to, end());
            _M_impl._M_size = __s;
-         }
 
-       if constexpr (__pocca)
-         _M_alloc = __other._M_alloc;
+           if constexpr (__pocca)
+             _M_alloc = __other._M_alloc;
+         }
 
        return *this;
       }
@@ -397,19 +401,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
          std::swap(_M_impl, __other._M_impl);
        else if (_M_alloc == __other._M_alloc)
          std::swap(_M_impl, __other._M_impl);
-       else
+       else if constexpr (__pocma)
          {
-           const auto __s = __other.size();
+           // Free current storage and take ownership of __other's storage.
+           _M_clear();
+           _M_impl = std::__exchange(__other._M_impl, {});
+         }
+       else // Allocators are unequal and don't propagate.
+         {
+           const size_type __s = __other.size();
 
-           if constexpr (__pocma || _M_impl._M_capacity < __s)
+           if (_M_impl._M_capacity < __s)
              {
                // Need to allocate new storage.
                _M_clear();
 
-               // Use the allocator we will have after this function returns.
-               auto& __alloc = __pocma ? __other._M_alloc : _M_alloc;
-               if (auto __f = _M_impl._M_allocate(__alloc, __s))
-                 std::uninitialized_copy_n(__other.begin(), __s, __f);
+               if (auto __f = _M_impl._M_allocate(_M_alloc, __s))
+                 {
+                   std::uninitialized_copy_n(__other.begin(), __s, __f);
+                   _M_impl._M_size = __s;
+                 }
              }
            else
              {
@@ -420,8 +431,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                auto __to = std::copy(__first, __mid, begin());
                __to = std::uninitialized_copy(__mid, __last, __to);
                std::destroy(__to, end());
+               _M_impl._M_size = __s;
              }
-           _M_impl._M_size = __s;
          }
 
        if constexpr (__pocma)
@@ -503,9 +514,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       swap(basic_stacktrace& __other) noexcept
       {
-       std::swap(_M_impl. __other._M_impl);
+       std::swap(_M_impl, __other._M_impl);
        if constexpr (_AllocTraits::propagate_on_container_swap::value)
          std::swap(_M_alloc, __other._M_alloc);
+       else if constexpr (!_AllocTraits::is_always_equal::value)
+         {
+           __glibcxx_assert(_M_alloc == __other._M_alloc);
+         }
       }
 
     private:
diff --git a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/stacktrace.cc b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/stacktrace.cc
new file mode 100644 (file)
index 0000000..8dfdf47
--- /dev/null
@@ -0,0 +1,215 @@
+// { dg-options "-std=gnu++23 -lstdc++_libbacktrace" }
+// { dg-do run { target c++23 } }
+// { dg-require-effective-target stacktrace }
+
+#include <stacktrace>
+#include "testsuite_allocator.h"
+
+static_assert( std::is_nothrow_default_constructible_v<std::stacktrace> );
+static_assert( std::is_copy_constructible_v<std::stacktrace> );
+static_assert( std::is_nothrow_move_constructible_v<std::stacktrace> );
+static_assert( std::is_copy_assignable_v<std::stacktrace> );
+static_assert( std::is_nothrow_move_assignable_v<std::stacktrace> );
+static_assert( std::is_nothrow_swappable_v<std::stacktrace> );
+
+void
+test_cons()
+{
+  {
+    using Stacktrace = std::stacktrace;
+    using Alloc = Stacktrace::allocator_type;
+
+    Stacktrace s0;
+    VERIFY( s0.empty() );
+    VERIFY( s0.size() == 0 );
+    VERIFY( s0.begin() == s0.end() );
+
+    Stacktrace s1(Alloc{});
+    VERIFY( s1.empty() );
+    VERIFY( s1.size() == 0 );
+    VERIFY( s1.begin() == s1.end() );
+
+    VERIFY( s0 == s1 );
+
+    Stacktrace s2(s0);
+    VERIFY( s2 == s0 );
+
+    const Stacktrace curr = Stacktrace::current();
+
+    Stacktrace s3(curr);
+    VERIFY( ! s3.empty() );
+    VERIFY( s3.size() != 0 );
+    VERIFY( s3.begin() != s3.end() );
+    VERIFY( s3 != s0 );
+
+    Stacktrace s4(s3);
+    VERIFY( ! s4.empty() );
+    VERIFY( s4.size() != 0 );
+    VERIFY( s4.begin() != s4.end() );
+    VERIFY( s4 == s3 );
+    VERIFY( s4 != s0 );
+
+    Stacktrace s5(std::move(s3));
+    VERIFY( ! s5.empty() );
+    VERIFY( s5.size() != 0 );
+    VERIFY( s5.begin() != s5.end() );
+    VERIFY( s5 == s4 );
+    VERIFY( s5 != s0 );
+    VERIFY( s3 == s0 );
+
+    Stacktrace s6(s4, Alloc{});
+    VERIFY( s6 == s4 );
+
+    Stacktrace s7(std::move(s6), Alloc{});
+    VERIFY( s7 == s4 );
+  }
+
+  {
+    using Alloc = __gnu_test::uneq_allocator<std::stacktrace_entry>;
+    using Stacktrace = std::basic_stacktrace<Alloc>;
+
+    Stacktrace s0;
+    VERIFY( s0.empty() );
+    VERIFY( s0.size() == 0 );
+    VERIFY( s0.begin() == s0.end() );
+
+    Stacktrace s1(Alloc{});
+    VERIFY( s1.empty() );
+    VERIFY( s1.size() == 0 );
+    VERIFY( s1.begin() == s1.end() );
+
+    VERIFY( s0 == s1 );
+
+    Stacktrace s2(s0);
+    VERIFY( s2 == s0 );
+
+    const Stacktrace curr = Stacktrace::current();
+
+    Stacktrace s3(curr);
+    VERIFY( ! s3.empty() );
+    VERIFY( s3.size() != 0 );
+    VERIFY( s3.begin() != s3.end() );
+    VERIFY( s3 != s0 );
+
+    Stacktrace s4(s3);
+    VERIFY( ! s4.empty() );
+    VERIFY( s4.size() != 0 );
+    VERIFY( s4.begin() != s4.end() );
+    VERIFY( s4 == s3 );
+    VERIFY( s4 != s0 );
+
+    Stacktrace s5(std::move(s3));
+    VERIFY( ! s5.empty() );
+    VERIFY( s5.size() != 0 );
+    VERIFY( s5.begin() != s5.end() );
+    VERIFY( s5 == s4 );
+    VERIFY( s5 != s0 );
+    VERIFY( s3 == s0 );
+
+    // TODO test allocator-extended copy/move
+
+    // TODO test allocator propagation
+  }
+}
+
+
+void
+test_assign()
+{
+  {
+    using Stacktrace = std::stacktrace;
+
+    Stacktrace s0;
+    s0 = s0;
+    VERIFY( s0.empty() );
+    s0 = std::move(s0);
+    VERIFY( s0.empty() );
+
+    Stacktrace s1 = Stacktrace::current();
+    VERIFY( s1 != s0 );
+    s0 = s1;
+    VERIFY( s0 == s1 );
+    VERIFY( s0.at(0).source_line() == (__LINE__ - 4) );
+
+    s1 = Stacktrace::current();
+    VERIFY( s1 != s0 );
+    Stacktrace s2 = s0;
+    Stacktrace s3 = s1;
+    s0 = std::move(s1);
+    VERIFY( s0 == s3 );
+    VERIFY( s1 == s2 ); // ISO C++: valid but unspecified; GCC: swapped.
+  }
+
+  {
+    using Alloc = __gnu_test::uneq_allocator<std::stacktrace_entry>;
+    using Stacktrace = std::basic_stacktrace<Alloc>;
+
+    Stacktrace s0;
+    s0 = s0;
+    VERIFY( s0.empty() );
+    s0 = std::move(s0);
+    VERIFY( s0.empty() );
+
+    Stacktrace s1 = Stacktrace::current();
+    VERIFY( s1 != s0 );
+    s0 = s1;
+    VERIFY( s0 == s1 );
+
+    s1 = Stacktrace::current(Alloc(__LINE__));
+    VERIFY( s1 != s0 );
+    s0 = std::move(s1);
+    VERIFY( s0.at(0).source_line() == s0.get_allocator().get_personality() );
+    VERIFY( s1.empty() ); // ISO C++: valid but unspecified; GCC: empty.
+
+    Stacktrace s2 = Stacktrace::current(s0.get_allocator());
+    Stacktrace s3 = s2;
+    s2 = std::move(s0);
+    VERIFY( s2.at(0).source_line() == s2.get_allocator().get_personality() );
+    VERIFY( s0 == s3 ); // ISO C++: valid but unspecified, GCC: swapped.
+  }
+}
+
+void
+test_swap()
+{
+  {
+    using Stacktrace = std::stacktrace;
+
+    Stacktrace s0;
+    Stacktrace s1 = Stacktrace::current();
+    swap(s0, s1);
+    VERIFY( s1.empty() );
+    VERIFY( ! s0.empty() );
+  }
+
+  {
+    using Alloc = __gnu_test::uneq_allocator<std::stacktrace_entry>;
+    using Stacktrace = std::basic_stacktrace<Alloc>;
+
+    Stacktrace s0;
+    Stacktrace s1 = Stacktrace::current();
+    swap(s0, s1);
+    VERIFY( s1.empty() );
+    VERIFY( ! s0.empty() );
+
+    // TODO test allocator propagation
+  }
+}
+
+void
+test_pr105031()
+{
+  // PR libstdc++/105031
+  // wrong constexpr if statement in operator=(basic_stacktrace&&)
+  using Alloc = __gnu_test::uneq_allocator<std::stacktrace_entry>;
+  std::basic_stacktrace<Alloc> s;
+  s = auto(s);
+}
+
+int main()
+{
+  test_cons();
+  test_assign();
+  test_swap();
+  test_pr105031();
+}