* Fwd: [PATCH] libstdc++: Add error handler for <stacktrace> [not found] <20221129214107.25572-1-gcc@hazardy.de> @ 2022-11-29 21:48 ` Björn Schäpers [not found] ` <CACb0b4kruVE28cZZh-ALpocSH+BdkYS_DXHC-iU+2MDm33SgNA@mail.gmail.com> [not found] ` <ee866216-8828-6b88-764d-9de8264108d5@gmail.com> 2 siblings, 0 replies; 9+ messages in thread From: Björn Schäpers @ 2022-11-29 21:48 UTC (permalink / raw) To: gcc-patches -------- Weitergeleitete Nachricht -------- Betreff: [PATCH] libstdc++: Add error handler for <stacktrace> Datum: Tue, 29 Nov 2022 22:41:07 +0100 Von: Björn Schäpers <gcc@hazardy.de> An: gcc-patches@gc.gnu.org, libstdc++@gcc.gnu.org From: Björn Schäpers <bjoern@hazardy.de> Not providing an error handler results in a nullpointer dereference when an error occurs. libstdc++-v3/ChangeLog * include/std/stacktrace: Add __backtrace_error_handler and use it in all calls to libbacktrace. --- libstdc++-v3/include/std/stacktrace | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace index e7cbbee5638..b786441cbad 100644 --- a/libstdc++-v3/include/std/stacktrace +++ b/libstdc++-v3/include/std/stacktrace @@ -85,6 +85,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #define __cpp_lib_stacktrace 202011L + inline void + __backtrace_error_handler(void*, const char*, int) {} + // [stacktrace.entry], class stacktrace_entry class stacktrace_entry { @@ -159,7 +162,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _S_init() { static __glibcxx_backtrace_state* __state - = __glibcxx_backtrace_create_state(nullptr, 1, nullptr, nullptr); + = __glibcxx_backtrace_create_state(nullptr, 1, + __backtrace_error_handler, nullptr); return __state; } @@ -192,7 +196,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __function != nullptr; }; const auto __state = _S_init(); - if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, nullptr, &__data)) + if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, + __backtrace_error_handler, &__data)) return true; if (__desc && __desc->empty()) { @@ -201,8 +206,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__symname) *static_cast<_Data*>(__data)->_M_desc = _S_demangle(__symname); }; - if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, nullptr, - &__data)) + if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, + __backtrace_error_handler, &__data)) return true; } return false; @@ -252,7 +257,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (auto __cb = __ret._M_prepare()) [[likely]] { auto __state = stacktrace_entry::_S_init(); - if (__glibcxx_backtrace_simple(__state, 1, __cb, nullptr, + if (__glibcxx_backtrace_simple(__state, 1, __cb, + __backtrace_error_handler, std::__addressof(__ret))) __ret._M_clear(); } @@ -270,7 +276,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (auto __cb = __ret._M_prepare()) [[likely]] { auto __state = stacktrace_entry::_S_init(); - if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, nullptr, + if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, + __backtrace_error_handler, std::__addressof(__ret))) __ret._M_clear(); } @@ -294,7 +301,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { auto __state = stacktrace_entry::_S_init(); int __err = __glibcxx_backtrace_simple(__state, __skip + 1, __cb, - nullptr, + __backtrace_error_handler, std::__addressof(__ret)); if (__err < 0) __ret._M_clear(); -- 2.38.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CACb0b4kruVE28cZZh-ALpocSH+BdkYS_DXHC-iU+2MDm33SgNA@mail.gmail.com>]
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> [not found] ` <CACb0b4kruVE28cZZh-ALpocSH+BdkYS_DXHC-iU+2MDm33SgNA@mail.gmail.com> @ 2022-11-30 13:05 ` Jonathan Wakely 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Wakely @ 2022-11-30 13:05 UTC (permalink / raw) To: Björn Schäpers; +Cc: libstdc++, gcc Patches [-- Attachment #1: Type: text/plain, Size: 889 bytes --] Resending with the typo in the mailing list address fixed... On Wed, 30 Nov 2022 at 12:31, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Tue, 29 Nov 2022 at 21:41, Björn Schäpers wrote: > > > > From: Björn Schäpers <bjoern@hazardy.de> > > > > Not providing an error handler results in a nullpointer dereference when > > an error occurs. > > > Thanks for the patch. This looks small enough to not require legal > paperwork, but if you intend to make further contributions (and I hope > you do!) please note the process at https://gcc.gnu.org/dco.html or > complete the paperwork for a copyright assignment to the FSF, > whichever you prefer. > > I'm going to test and commit the attached patch, which replaces your > __backtrace_error_handler with a static member function so that we > don't make the name visible in the global namespace. > > Thanks again! [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 3161 bytes --] commit 0e32de31f34a88b941acd2f471ba6e8e945372cf Author: Björn Schäpers <bjoern@hazardy.de> Date: Wed Nov 30 12:04:16 2022 libstdc++: Add error handler for <stacktrace> Not providing an error handler results in a null pointer dereference when an error occurs. Co-authored-by: Jonathan Wakely <jwakely@redhat.com> libstdc++-v3/ChangeLog: * include/std/stacktrace (stacktrace_entry::_S_err_handler): New static function. (stacktrace_entry, basic_stacktrace): Pass &_S_err_handler to all calls to libbacktrace. diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace index e7cbbee5638..ec3335e89d8 100644 --- a/libstdc++-v3/include/std/stacktrace +++ b/libstdc++-v3/include/std/stacktrace @@ -155,11 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<typename _Allocator> friend class basic_stacktrace; + static void _S_err_handler(void*, const char*, int) { } + static __glibcxx_backtrace_state* _S_init() { static __glibcxx_backtrace_state* __state - = __glibcxx_backtrace_create_state(nullptr, 1, nullptr, nullptr); + = __glibcxx_backtrace_create_state(nullptr, 1, _S_err_handler, nullptr); return __state; } @@ -192,7 +194,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __function != nullptr; }; const auto __state = _S_init(); - if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, nullptr, &__data)) + if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, _S_err_handler, + &__data)) return true; if (__desc && __desc->empty()) { @@ -201,8 +204,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__symname) *static_cast<_Data*>(__data)->_M_desc = _S_demangle(__symname); }; - if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, nullptr, - &__data)) + if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, + _S_err_handler, &__data)) return true; } return false; @@ -252,7 +255,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (auto __cb = __ret._M_prepare()) [[likely]] { auto __state = stacktrace_entry::_S_init(); - if (__glibcxx_backtrace_simple(__state, 1, __cb, nullptr, + if (__glibcxx_backtrace_simple(__state, 1, __cb, + stacktrace_entry::_S_err_handler, std::__addressof(__ret))) __ret._M_clear(); } @@ -270,7 +274,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (auto __cb = __ret._M_prepare()) [[likely]] { auto __state = stacktrace_entry::_S_init(); - if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, nullptr, + if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, + stacktrace_entry::_S_err_handler, std::__addressof(__ret))) __ret._M_clear(); } @@ -294,7 +299,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { auto __state = stacktrace_entry::_S_init(); int __err = __glibcxx_backtrace_simple(__state, __skip + 1, __cb, - nullptr, + stacktrace_entry::_S_err_handler, std::__addressof(__ret)); if (__err < 0) __ret._M_clear(); ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <ee866216-8828-6b88-764d-9de8264108d5@gmail.com>]
[parent not found: <CACb0b4nR8Rx9=vpaTh48EkrL+osj8o7FTaBchPW8AUaQVxf0SA@mail.gmail.com>]
[parent not found: <CACb0b4kWc8izwDr_H6iCq1cJLwAr+d9ViwVnpYFy=s6rkiP55g@mail.gmail.com>]
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> [not found] ` <CACb0b4kWc8izwDr_H6iCq1cJLwAr+d9ViwVnpYFy=s6rkiP55g@mail.gmail.com> @ 2022-11-30 13:07 ` Jonathan Wakely 2022-11-30 18:00 ` François Dumont 2022-11-30 18:17 ` François Dumont 0 siblings, 2 replies; 9+ messages in thread From: Jonathan Wakely @ 2022-11-30 13:07 UTC (permalink / raw) To: François Dumont; +Cc: Björn Schäpers, libstdc++, gcc Patches [-- Attachment #1: Type: text/plain, Size: 1036 bytes --] On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> >> >> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: >>> >>> Good catch, then we also need this patch. >> >> >> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. >> >> We also need to pass an error handler to the __glibcxx_backtrace_create_state call in formatter.h. >> >> Now that I look at this code again, why do we need the _M_backtrace_full member? It's always set to the same thing, why can't we just call that function directly? > > > Oh right, I remember now ... because otherwise the libstdc++.so library needs the definition of __glibcxx_backtrace_full. I'm testing the attached patch. > >> >> And I think we should use threaded=1 for the __glibcxx_backtrace_create_state call. >> >> So like the attached patch. >> >> [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 3502 bytes --] commit 6c9cc05dc097f6ee66f18731a6247cce36823d54 Author: Jonathan Wakely <jwakely@redhat.com> Date: Wed Nov 30 12:32:53 2022 libstdc++: Pass error handler to libbacktrace functions Also pass threaded=1 to __glibcxx_backtrace_create_state and remove some of the namespace scope declarations. libstdc++-v3/ChangeLog: * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE] (_Error_formatter::_Error_formatter): Pass error handler to __glibcxx_backtrace_create_state. Pass 1 for threaded argument. (_Error_formatter::_S_err): Define empty function. * src/c++11/debug.cc (_Error_formatter::_M_error): Pass error handler to __glibcxx_backtrace_full. diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h index f120163c6d4..e8a83a21bde 100644 --- a/libstdc++-v3/include/debug/formatter.h +++ b/libstdc++-v3/include/debug/formatter.h @@ -32,32 +32,17 @@ #include <bits/c++config.h> #if _GLIBCXX_HAVE_STACKTRACE -struct __glibcxx_backtrace_state; - extern "C" { - __glibcxx_backtrace_state* + struct __glibcxx_backtrace_state* __glibcxx_backtrace_create_state(const char*, int, void(*)(void*, const char*, int), void*); - - typedef int (*__glibcxx_backtrace_full_callback) ( - void*, __UINTPTR_TYPE__, const char *, int, const char*); - - typedef void (*__glibcxx_backtrace_error_callback) ( - void*, const char*, int); - - typedef int (*__glibcxx_backtrace_full_func) ( - __glibcxx_backtrace_state*, int, - __glibcxx_backtrace_full_callback, - __glibcxx_backtrace_error_callback, - void*); - int __glibcxx_backtrace_full( - __glibcxx_backtrace_state*, int, - __glibcxx_backtrace_full_callback, - __glibcxx_backtrace_error_callback, + struct __glibcxx_backtrace_state*, int, + int (*)(void*, __UINTPTR_TYPE__, const char *, int, const char*), + void (*)(void*, const char*, int), void*); } #endif @@ -609,10 +594,10 @@ namespace __gnu_debug , _M_function(__function) #if _GLIBCXX_HAVE_STACKTRACE # ifdef _GLIBCXX_DEBUG_BACKTRACE - , _M_backtrace_state(__glibcxx_backtrace_create_state(0, 0, 0, 0)) + , _M_backtrace_state(__glibcxx_backtrace_create_state(0, 1, _S_err, 0)) , _M_backtrace_full(&__glibcxx_backtrace_full) # else - , _M_backtrace_state() + , _M_backtrace_state(0) # endif #endif { } @@ -631,8 +616,12 @@ namespace __gnu_debug const char* _M_text; const char* _M_function; #if _GLIBCXX_HAVE_STACKTRACE - __glibcxx_backtrace_state* _M_backtrace_state; - __glibcxx_backtrace_full_func _M_backtrace_full; + struct __glibcxx_backtrace_state* _M_backtrace_state; + // TODO: Remove _M_backtrace_full after __glibcxx_backtrace_full is moved + // from libstdc++_libbacktrace.a to libstdc++.so: + __decltype(&__glibcxx_backtrace_full) _M_backtrace_full; + + static void _S_err(void*, const char*, int) { } #endif public: diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index 9eda38023f7..c08eaa7f921 100644 --- a/libstdc++-v3/src/c++11/debug.cc +++ b/libstdc++-v3/src/c++11/debug.cc @@ -1193,7 +1193,7 @@ namespace __gnu_debug { print_literal(ctx, "Backtrace:\n"); _M_backtrace_full( - _M_backtrace_state, 1, print_backtrace, nullptr, &ctx); + _M_backtrace_state, 1, print_backtrace, _S_err, &ctx); ctx._M_first_line = true; print_literal(ctx, "\n"); } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> 2022-11-30 13:07 ` Jonathan Wakely @ 2022-11-30 18:00 ` François Dumont 2022-12-06 21:44 ` Jonathan Wakely 2022-11-30 18:17 ` François Dumont 1 sibling, 1 reply; 9+ messages in thread From: François Dumont @ 2022-11-30 18:00 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Björn Schäpers, libstdc++, gcc Patches On 30/11/22 14:07, Jonathan Wakely wrote: > On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> >> On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: >>> >>> >>> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: >>>> Good catch, then we also need this patch. >>> >>> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. No strong opinion on that but if we do not print anything the output will be: Backtrace: Error: ... I just considered that it did not cost much to report the issue to the user that defined _GLIBCXX_DEBUG_BACKTRACE and so is expecting a backtrace. Maybe printing "Backtrace:\n" could be done in the normal callback leaving the user with the feeling that _GLIBCXX_DEBUG_BACKTRACE does not work. >>> >>> We also need to pass an error handler to the __glibcxx_backtrace_create_state call in formatter.h. >>> >>> Now that I look at this code again, why do we need the _M_backtrace_full member? It's always set to the same thing, why can't we just call that function directly? >> >> Oh right, I remember now ... because otherwise the libstdc++.so library needs the definition of __glibcxx_backtrace_full. > I'm testing the attached patch. > > >>> And I think we should use threaded=1 for the __glibcxx_backtrace_create_state call. >>> >>> So like the attached patch. >>> >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> 2022-11-30 18:00 ` François Dumont @ 2022-12-06 21:44 ` Jonathan Wakely 2022-12-07 17:58 ` François Dumont 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2022-12-06 21:44 UTC (permalink / raw) To: François Dumont; +Cc: Björn Schäpers, libstdc++, gcc Patches [-- Attachment #1: Type: text/plain, Size: 1068 bytes --] On Wed, 30 Nov 2022 at 18:00, François Dumont <frs.dumont@gmail.com> wrote: > > On 30/11/22 14:07, Jonathan Wakely wrote: > > On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: > >> > >> > >> On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: > >>> > >>> > >>> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > >>>> Good catch, then we also need this patch. > >>> > >>> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. > > No strong opinion on that but if we do not print anything the output > will be: > > Backtrace: > > Error: ... > > I just considered that it did not cost much to report the issue to the > user that defined _GLIBCXX_DEBUG_BACKTRACE and so is expecting a backtrace. > > Maybe printing "Backtrace:\n" could be done in the normal callback > leaving the user with the feeling that _GLIBCXX_DEBUG_BACKTRACE does not > work. OK, how's this? Tested x86_64-linux. [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 4115 bytes --] commit aadf83283f0d3e1d473ac17595ba73d0210bb9ba Author: Jonathan Wakely <jwakely@redhat.com> Date: Wed Nov 30 12:32:53 2022 libstdc++: Pass error handler to libbacktrace functions Also pass threaded=1 to __glibcxx_backtrace_create_state and remove some of the namespace scope declarations in the header. Co-authored-by: François Dumont <frs.dumont@gmail.com> libstdc++-v3/ChangeLog: * include/debug/formatter.h [_GLIBCXX_DEBUG_BACKTRACE] (_Error_formatter::_Error_formatter): Pass error handler to __glibcxx_backtrace_create_state. Pass 1 for threaded argument. (_Error_formatter::_S_err): Define empty function. * src/c++11/debug.cc (_Error_formatter::_M_error): Pass error handler to __glibcxx_backtrace_full. diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h index f120163c6d4..e8a83a21bde 100644 --- a/libstdc++-v3/include/debug/formatter.h +++ b/libstdc++-v3/include/debug/formatter.h @@ -32,32 +32,17 @@ #include <bits/c++config.h> #if _GLIBCXX_HAVE_STACKTRACE -struct __glibcxx_backtrace_state; - extern "C" { - __glibcxx_backtrace_state* + struct __glibcxx_backtrace_state* __glibcxx_backtrace_create_state(const char*, int, void(*)(void*, const char*, int), void*); - - typedef int (*__glibcxx_backtrace_full_callback) ( - void*, __UINTPTR_TYPE__, const char *, int, const char*); - - typedef void (*__glibcxx_backtrace_error_callback) ( - void*, const char*, int); - - typedef int (*__glibcxx_backtrace_full_func) ( - __glibcxx_backtrace_state*, int, - __glibcxx_backtrace_full_callback, - __glibcxx_backtrace_error_callback, - void*); - int __glibcxx_backtrace_full( - __glibcxx_backtrace_state*, int, - __glibcxx_backtrace_full_callback, - __glibcxx_backtrace_error_callback, + struct __glibcxx_backtrace_state*, int, + int (*)(void*, __UINTPTR_TYPE__, const char *, int, const char*), + void (*)(void*, const char*, int), void*); } #endif @@ -609,10 +594,10 @@ namespace __gnu_debug , _M_function(__function) #if _GLIBCXX_HAVE_STACKTRACE # ifdef _GLIBCXX_DEBUG_BACKTRACE - , _M_backtrace_state(__glibcxx_backtrace_create_state(0, 0, 0, 0)) + , _M_backtrace_state(__glibcxx_backtrace_create_state(0, 1, _S_err, 0)) , _M_backtrace_full(&__glibcxx_backtrace_full) # else - , _M_backtrace_state() + , _M_backtrace_state(0) # endif #endif { } @@ -631,8 +616,12 @@ namespace __gnu_debug const char* _M_text; const char* _M_function; #if _GLIBCXX_HAVE_STACKTRACE - __glibcxx_backtrace_state* _M_backtrace_state; - __glibcxx_backtrace_full_func _M_backtrace_full; + struct __glibcxx_backtrace_state* _M_backtrace_state; + // TODO: Remove _M_backtrace_full after __glibcxx_backtrace_full is moved + // from libstdc++_libbacktrace.a to libstdc++.so: + __decltype(&__glibcxx_backtrace_full) _M_backtrace_full; + + static void _S_err(void*, const char*, int) { } #endif public: diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index 9eda38023f7..83996ffe622 100644 --- a/libstdc++-v3/src/c++11/debug.cc +++ b/libstdc++-v3/src/c++11/debug.cc @@ -1141,6 +1141,23 @@ namespace return ret; } + + void + print_backtrace_error(void* data, const char* msg, int errnum) + { + PrintContext& ctx = *static_cast<PrintContext*>(data); + + print_literal(ctx, "Backtrace unavailable: "); + print_word(ctx, msg ? msg : "<unknown error>"); + if (errnum > 0) + { + char buf[64]; + int written = __builtin_sprintf(buf, " (errno=%d)\n", errnum); + print_word(ctx, buf, written); + } + else + print_literal(ctx, "\n"); + } #endif } @@ -1193,7 +1210,7 @@ namespace __gnu_debug { print_literal(ctx, "Backtrace:\n"); _M_backtrace_full( - _M_backtrace_state, 1, print_backtrace, nullptr, &ctx); + _M_backtrace_state, 1, print_backtrace, print_backtrace_error, &ctx); ctx._M_first_line = true; print_literal(ctx, "\n"); } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> 2022-12-06 21:44 ` Jonathan Wakely @ 2022-12-07 17:58 ` François Dumont 2022-12-07 20:06 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: François Dumont @ 2022-12-07 17:58 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Björn Schäpers, libstdc++, gcc Patches Looks perfect to me, thanks. On 06/12/22 22:44, Jonathan Wakely wrote: > On Wed, 30 Nov 2022 at 18:00, François Dumont <frs.dumont@gmail.com> wrote: >> On 30/11/22 14:07, Jonathan Wakely wrote: >>> On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: >>>> >>>> On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: >>>>> >>>>> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: >>>>>> Good catch, then we also need this patch. >>>>> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. >> No strong opinion on that but if we do not print anything the output >> will be: >> >> Backtrace: >> >> Error: ... >> >> I just considered that it did not cost much to report the issue to the >> user that defined _GLIBCXX_DEBUG_BACKTRACE and so is expecting a backtrace. >> >> Maybe printing "Backtrace:\n" could be done in the normal callback >> leaving the user with the feeling that _GLIBCXX_DEBUG_BACKTRACE does not >> work. > OK, how's this? > > Tested x86_64-linux. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> 2022-12-07 17:58 ` François Dumont @ 2022-12-07 20:06 ` Jonathan Wakely 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Wakely @ 2022-12-07 20:06 UTC (permalink / raw) To: François Dumont; +Cc: Björn Schäpers, libstdc++, gcc Patches On Wed, 7 Dec 2022 at 17:58, François Dumont <frs.dumont@gmail.com> wrote: > > Looks perfect to me, thanks. OK thanks, it's pushed to trunk now. > > On 06/12/22 22:44, Jonathan Wakely wrote: > > On Wed, 30 Nov 2022 at 18:00, François Dumont <frs.dumont@gmail.com> wrote: > >> On 30/11/22 14:07, Jonathan Wakely wrote: > >>> On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: > >>>> > >>>> On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: > >>>>> > >>>>> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > >>>>>> Good catch, then we also need this patch. > >>>>> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. > >> No strong opinion on that but if we do not print anything the output > >> will be: > >> > >> Backtrace: > >> > >> Error: ... > >> > >> I just considered that it did not cost much to report the issue to the > >> user that defined _GLIBCXX_DEBUG_BACKTRACE and so is expecting a backtrace. > >> > >> Maybe printing "Backtrace:\n" could be done in the normal callback > >> leaving the user with the feeling that _GLIBCXX_DEBUG_BACKTRACE does not > >> work. > > OK, how's this? > > > > Tested x86_64-linux. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> 2022-11-30 13:07 ` Jonathan Wakely 2022-11-30 18:00 ` François Dumont @ 2022-11-30 18:17 ` François Dumont 1 sibling, 0 replies; 9+ messages in thread From: François Dumont @ 2022-11-30 18:17 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Björn Schäpers, libstdc++, gcc Patches On 30/11/22 14:07, Jonathan Wakely wrote: > On Wed, 30 Nov 2022 at 11:57, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> >> On Wed, 30 Nov 2022 at 11:54, Jonathan Wakely <jwakely@redhat.com> wrote: >>> >>> >>> On Wed, 30 Nov 2022 at 06:04, François Dumont via Libstdc++ <libstdc++@gcc.gnu.org> wrote: >>>> Good catch, then we also need this patch. >>> >>> Is it worth printing an error? If we can't show the backtrace because of an error, we can just print nothing there. >>> >>> We also need to pass an error handler to the __glibcxx_backtrace_create_state call in formatter.h. >>> >>> Now that I look at this code again, why do we need the _M_backtrace_full member? It's always set to the same thing, why can't we just call that function directly? >> >> Oh right, I remember now ... because otherwise the libstdc++.so library needs the definition of __glibcxx_backtrace_full. > I'm testing the attached patch. > > >>> And I think we should use threaded=1 for the __glibcxx_backtrace_create_state call. You mean that 2 threads could try to assert at the same time. I don't know what's the rule on the static _Error_formatter instance in _S_at. If we have a strong guaranty that only 1 instance will be created then I understand why we need threaded=1. Even if in this case the 2 threads will report the same stacktrace. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libstdc++: Add error handler for <stacktrace> [not found] ` <ee866216-8828-6b88-764d-9de8264108d5@gmail.com> [not found] ` <CACb0b4nR8Rx9=vpaTh48EkrL+osj8o7FTaBchPW8AUaQVxf0SA@mail.gmail.com> @ 2022-11-30 19:20 ` Björn Schäpers 1 sibling, 0 replies; 9+ messages in thread From: Björn Schäpers @ 2022-11-30 19:20 UTC (permalink / raw) To: François Dumont, gcc-patches, libstdc++ One could (for a manual test) always change libbacktrace to call the callback. Or invoke it on a platform where libbacktrace can't figure out the executable path on its own, like currently windows. As for an automated test, I have no idea how to enforce that, without changing the code to be tested. Björn. Am 30.11.2022 um 07:04 schrieb François Dumont: > Good catch, then we also need this patch. > > I still need to test it thought, just to make sure it compiles. Unless you have > a nice way to force call to the error callback ? > > François > > On 29/11/22 22:41, Björn Schäpers wrote: >> From: Björn Schäpers <bjoern@hazardy.de> >> >> Not providing an error handler results in a nullpointer dereference when >> an error occurs. >> >> libstdc++-v3/ChangeLog >> >> * include/std/stacktrace: Add __backtrace_error_handler and use >> it in all calls to libbacktrace. >> --- >> libstdc++-v3/include/std/stacktrace | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/libstdc++-v3/include/std/stacktrace >> b/libstdc++-v3/include/std/stacktrace >> index e7cbbee5638..b786441cbad 100644 >> --- a/libstdc++-v3/include/std/stacktrace >> +++ b/libstdc++-v3/include/std/stacktrace >> @@ -85,6 +85,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> #define __cpp_lib_stacktrace 202011L >> + inline void >> + __backtrace_error_handler(void*, const char*, int) {} >> + >> // [stacktrace.entry], class stacktrace_entry >> class stacktrace_entry >> { >> @@ -159,7 +162,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> _S_init() >> { >> static __glibcxx_backtrace_state* __state >> - = __glibcxx_backtrace_create_state(nullptr, 1, nullptr, nullptr); >> + = __glibcxx_backtrace_create_state(nullptr, 1, >> + __backtrace_error_handler, nullptr); >> return __state; >> } >> @@ -192,7 +196,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> return __function != nullptr; >> }; >> const auto __state = _S_init(); >> - if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, nullptr, &__data)) >> + if (::__glibcxx_backtrace_pcinfo(__state, _M_pc, +__cb, >> + __backtrace_error_handler, &__data)) >> return true; >> if (__desc && __desc->empty()) >> { >> @@ -201,8 +206,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> if (__symname) >> *static_cast<_Data*>(__data)->_M_desc = _S_demangle(__symname); >> }; >> - if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, nullptr, >> - &__data)) >> + if (::__glibcxx_backtrace_syminfo(__state, _M_pc, +__cb2, >> + __backtrace_error_handler, &__data)) >> return true; >> } >> return false; >> @@ -252,7 +257,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> if (auto __cb = __ret._M_prepare()) [[likely]] >> { >> auto __state = stacktrace_entry::_S_init(); >> - if (__glibcxx_backtrace_simple(__state, 1, __cb, nullptr, >> + if (__glibcxx_backtrace_simple(__state, 1, __cb, >> + __backtrace_error_handler, >> std::__addressof(__ret))) >> __ret._M_clear(); >> } >> @@ -270,7 +276,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> if (auto __cb = __ret._M_prepare()) [[likely]] >> { >> auto __state = stacktrace_entry::_S_init(); >> - if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, nullptr, >> + if (__glibcxx_backtrace_simple(__state, __skip + 1, __cb, >> + __backtrace_error_handler, >> std::__addressof(__ret))) >> __ret._M_clear(); >> } >> @@ -294,7 +301,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> { >> auto __state = stacktrace_entry::_S_init(); >> int __err = __glibcxx_backtrace_simple(__state, __skip + 1, __cb, >> - nullptr, >> + __backtrace_error_handler, >> std::__addressof(__ret)); >> if (__err < 0) >> __ret._M_clear(); > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-12-07 20:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20221129214107.25572-1-gcc@hazardy.de>
2022-11-29 21:48 ` Fwd: [PATCH] libstdc++: Add error handler for <stacktrace> Björn Schäpers
[not found] ` <CACb0b4kruVE28cZZh-ALpocSH+BdkYS_DXHC-iU+2MDm33SgNA@mail.gmail.com>
2022-11-30 13:05 ` Jonathan Wakely
[not found] ` <ee866216-8828-6b88-764d-9de8264108d5@gmail.com>
[not found] ` <CACb0b4nR8Rx9=vpaTh48EkrL+osj8o7FTaBchPW8AUaQVxf0SA@mail.gmail.com>
[not found] ` <CACb0b4kWc8izwDr_H6iCq1cJLwAr+d9ViwVnpYFy=s6rkiP55g@mail.gmail.com>
2022-11-30 13:07 ` Jonathan Wakely
2022-11-30 18:00 ` François Dumont
2022-12-06 21:44 ` Jonathan Wakely
2022-12-07 17:58 ` François Dumont
2022-12-07 20:06 ` Jonathan Wakely
2022-11-30 18:17 ` François Dumont
2022-11-30 19:20 ` Björn Schäpers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).