diff mbox series

[committed] libstdc++: Improve behaviour of std::stacktrace::current

Message ID 20220411170124.169564-1-jwakely@redhat.com
State New
Headers show
Series [committed] libstdc++: Improve behaviour of std::stacktrace::current | expand

Commit Message

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

-- >8 --

This prevents inlining the current() function to guarantee that it is
present in the stacktrace, then tells libbacktrace to skip that frame.

To avoid overflow in the int argument to __glibcxx_backtrace_simple, we
need to check if the skip parameter exceeds INT_MAX (which is possible
for 16-bit targets where short and int have the same width). We also
need to limit the size of the returned value to the max_depth parameter,
which was missing previously.

This also fixes basic_stacktrace::max_size() to not exceed the maximum
size supported by the allocator, which might be smaller than the maximum
value of size_type.

libstdc++-v3/ChangeLog:

	* include/std/stacktrace (basic_stacktrace::current): Duplicate
	implementation into each overload. Add noinline attribute and
	skip current frame.
	(basic_stacktrace::max_size()): Call _Impl::_S_max_size.
	(basic_stacktrace::_S_curr_cb()): New function defining lambda.
	(basic_stacktrace::_Impl::_S_max_size): New function defining
	maximum size in terms of allocator and size_type.
	(basic_stacktrace::_Impl::_M_allocate): Check against
	max_size().
	* testsuite/19_diagnostics/stacktrace/entry.cc: Call function
	for non-constexpr checks. Check line number is correct.
---
 libstdc++-v3/include/std/stacktrace           | 91 ++++++++++++++-----
 .../19_diagnostics/stacktrace/entry.cc        |  7 +-
 2 files changed, 73 insertions(+), 25 deletions(-)

Comments

Jonathan Wakely April 11, 2022, 9:27 p.m. UTC | #1 
On Mon, 11 Apr 2022 at 18:03, Jonathan Wakely via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>         // Precondition: _M_frames == nullptr
>         pointer
>         _M_allocate(allocator_type& __alloc, size_type __n) noexcept
>         {
>           __try
>             {
> -             _M_frames = __n ? __alloc.allocate(__n) : nullptr;
> -             _M_capacity = __n;
> +             if (0 < __n && __n <= _S_max_size(__alloc)) [[unlikely]]

I originally changed this to return early if the size isn't OK:

  if (unlikely condition)
    return nullptr;

but in the version I pushed it's:

  if (likely condition)
    // do allocation

but forgot to change the attribute to match.

Fixed by the attached. Tested x86_64-linux and pushed to trunk.

I have further improvements to stacktrace::current coming tomorrow.
commit b1124648ff8f655307f264d7b353fd68e3b03e9c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Apr 11 20:13:44 2022

    libstdc++: Fix incorrect branch prediction hint in std::stacktrace
    
    libstdc++-v3/ChangeLog:
    
            * include/std/stacktrace (basic_stacktrace::_Impl::_M_allocate):
            Change [[unlikely]] attribute to [[likely]].

diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace
index dd78c71c5dc..79038e803f2 100644
--- a/libstdc++-v3/include/std/stacktrace
+++ b/libstdc++-v3/include/std/stacktrace
@@ -579,7 +579,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	{
 	  __try
 	    {
-	      if (0 < __n && __n <= _S_max_size(__alloc)) [[unlikely]]
+	      if (0 < __n && __n <= _S_max_size(__alloc)) [[likely]]
 		{
 		  _M_frames = __alloc.allocate(__n);
 		  _M_capacity = __n;
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace
index 623f44bdca4..4e271cef3f3 100644
--- a/libstdc++-v3/include/std/stacktrace
+++ b/libstdc++-v3/include/std/stacktrace
@@ -36,6 +36,7 @@ 
 #include <bits/stl_algo.h>
 #include <bits/stl_iterator.h>
 #include <bits/stl_uninitialized.h>
+#include <ext/numeric_traits.h>
 #include <cxxabi.h>
 
 struct __glibcxx_backtrace_state;
@@ -232,19 +233,42 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       // [stacktrace.basic.ctor], creation and assignment
 
+      [[__gnu__::__noinline__]]
       static basic_stacktrace
       current(const allocator_type& __alloc = allocator_type()) noexcept
       {
-	return current(0, size_type(-1), __alloc);
+	auto __state = stacktrace_entry::_S_init();
+	basic_stacktrace __ret(__alloc);
+	if (!__ret._M_reserve(64)) [[unlikely]]
+	  return __ret;
+
+	if (__glibcxx_backtrace_simple(__state, 1, _S_curr_cb(),
+				       nullptr, std::__addressof(__ret)))
+	  __ret._M_clear();
+
+	return __ret;
       }
 
+      [[__gnu__::__noinline__]]
       static basic_stacktrace
       current(size_type __skip,
 	      const allocator_type& __alloc = allocator_type()) noexcept
       {
-	return current(__skip, size_type(-1), __alloc);
+	auto __state = stacktrace_entry::_S_init();
+	basic_stacktrace __ret(__alloc);
+	if (__skip >= __INT_MAX__) [[unlikely]]
+	  return __ret;
+	if (!__ret._M_reserve(64)) [[unlikely]]
+	  return __ret;
+
+	if (__glibcxx_backtrace_simple(__state, __skip + 1, _S_curr_cb(),
+				       nullptr, std::__addressof(__ret)))
+	  __ret._M_clear();
+
+	return __ret;
       }
 
+      [[__gnu__::__noinline__]]
       static basic_stacktrace
       current(size_type __skip, size_type __max_depth,
 	      const allocator_type& __alloc = allocator_type()) noexcept
@@ -253,23 +277,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 	auto __state = stacktrace_entry::_S_init();
 	basic_stacktrace __ret(__alloc);
-	if (!__ret._M_reserve(std::min<int>(__max_depth, 64)))
+	if (__max_depth == 0 || __skip >= __INT_MAX__) [[unlikely]]
+	  return __ret;
+	if (!__ret._M_reserve(std::min<int>(__max_depth, 64))) [[unlikely]]
 	  return __ret;
 
-	auto __cb = [](void* __data, uintptr_t __pc) {
-	  auto& __s = *static_cast<basic_stacktrace*>(__data);
-	  stacktrace_entry __f;
-	  __f._M_pc = __pc;
-	  if (__s._M_push_back(__f))
-	    return 0;
-	  return 1;
-	};
+	if (__glibcxx_backtrace_simple(__state, __skip + 1, _S_curr_cb(),
+				       nullptr, std::__addressof(__ret)))
+	  __ret._M_clear();
+	else if (__ret.size() > __max_depth)
+	  __ret.resize(__max_depth);
 
-	if (__glibcxx_backtrace_simple(__state, __skip, +__cb, nullptr,
-				       std::__addressof(__ret)))
-	  {
-	    __ret._M_clear();
-	  }
 	return __ret;
       }
 
@@ -443,7 +461,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       [[nodiscard]] bool empty() const noexcept { return size() == 0; }
       size_type size() const noexcept { return _M_impl._M_size; }
-      size_type max_size() const noexcept { return size_type(-1); }
+
+      size_type
+      max_size() const noexcept
+      { return _Impl::_S_max_size(_M_impl._M_alloc); }
 
       const_reference
       operator[](size_type __n) const noexcept
@@ -507,6 +528,20 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	_M_impl._M_deallocate(_M_alloc);
       }
 
+      static auto
+      _S_curr_cb() noexcept
+      -> int (*) (void*, uintptr_t)
+      {
+	return [](void* __data, uintptr_t __pc) {
+	  auto& __s = *static_cast<basic_stacktrace*>(__data);
+	  stacktrace_entry __f;
+	  __f._M_pc = __pc;
+	  if (__s._M_push_back(__f))
+	    return 0;
+	  return 1;
+	};
+      }
+
       struct _Impl
       {
 	using pointer = typename _AllocTraits::pointer;
@@ -515,21 +550,33 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	size_type _M_size     = 0;
 	size_type _M_capacity = 0;
 
+	static size_type
+	_S_max_size(const allocator_type& __alloc) noexcept
+	{
+	  const size_t __size_max = __gnu_cxx::__int_traits<size_type>::__max;
+	  const size_t __alloc_max = _AllocTraits::max_size(__alloc);
+	  return std::min(__size_max, __alloc_max);
+	}
+
 	// Precondition: _M_frames == nullptr
 	pointer
 	_M_allocate(allocator_type& __alloc, size_type __n) noexcept
 	{
 	  __try
 	    {
-	      _M_frames = __n ? __alloc.allocate(__n) : nullptr;
-	      _M_capacity = __n;
+	      if (0 < __n && __n <= _S_max_size(__alloc)) [[unlikely]]
+		{
+		  _M_frames = __alloc.allocate(__n);
+		  _M_capacity = __n;
+		  return _M_frames;
+		}
 	    }
 	  __catch (...)
 	    {
-	      _M_frames = nullptr;
-	      _M_capacity = 0;
 	    }
-	  return _M_frames;
+	  _M_frames = nullptr;
+	  _M_capacity = 0;
+	  return nullptr;;
 	}
 
 	void
diff --git a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/entry.cc b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/entry.cc
index 0bbcabd8fae..a222c425b20 100644
--- a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/entry.cc
+++ b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/entry.cc
@@ -36,7 +36,8 @@  test_members()
   VERIFY( e1 != e2 );
   VERIFY( e1.description() == e2.description() );
   VERIFY( e1.source_file() == e2.source_file() );
-  VERIFY( e1.source_line() != e2.source_line() );
+  VERIFY( e1.source_line() == (__LINE__ - 5) );
+  VERIFY( e2.source_line() == (__LINE__ - 5) );
 
   std::stacktrace_entry e3 = []{
     return std::stacktrace::current().at(0);
@@ -44,10 +45,10 @@  test_members()
   VERIFY( e1 != e3 );
   VERIFY( e1.description() != e3.description() );
   VERIFY( e1.source_file() == e3.source_file() );
-  VERIFY( e1.source_line() != e3.source_line() );
+  VERIFY( e3.source_line() == (__LINE__ - 5) );
 }
 
 int main()
 {
-  test_constexpr();
+  test_members();
 }