diff mbox series

[committed] libstdc++: Fix std::basic_stacktrace special members [PR105031]

Message ID 20220411170133.169581-1-jwakely@redhat.com
State New
Headers show
Series [committed] libstdc++: Fix std::basic_stacktrace special members [PR105031] | expand

Commit Message

Jonathan Wakely April 11, 2022, 5:01 p.m. UTC 
Tested x86_64-linux, pushed to trunk.

-- >8 --

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           |  59 +++--
 .../19_diagnostics/stacktrace/stacktrace.cc   | 215 ++++++++++++++++++
 2 files changed, 252 insertions(+), 22 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/19_diagnostics/stacktrace/stacktrace.cc
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace
index 4e271cef3f3..dd78c71c5dc 100644
--- a/libstdc++-v3/include/std/stacktrace
+++ b/libstdc++-v3/include/std/stacktrace
@@ -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
index 00000000000..8dfdf4739be
--- /dev/null
+++ b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/stacktrace.cc
@@ -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();
+}