On Thu, Oct 16, 2025 at 3:03 PM Jonathan Wakely <[email protected]> wrote:
> > > On Thu, 16 Oct 2025 at 13:22, Tomasz Kaminski <[email protected]> wrote: > > > > > > > > On Thu, Oct 16, 2025 at 2:13 PM Jonathan Wakely <[email protected]> > wrote: > >> > >> On Thu, 16 Oct 2025 at 12:53, Tomasz Kaminski <[email protected]> > wrote: > >> > > >> > > >> > > >> > On Thu, Oct 16, 2025 at 1:24 PM Jonathan Wakely <[email protected]> > wrote: > >> >> > >> >> With this change locations that don't have a source file look like > this: > >> >> > >> >> 18# __libc_start_call_main at [0x7fd6568f6574] > >> >> 19# __libc_start_main@GLIBC_2.2.5 at [0x7fd6568f6627] > >> >> 20# _start at [0x4006a4] > >> >> > >> >> Instead of: > >> >> > >> >> 18# __libc_start_call_main at :0 > >> >> 19# __libc_start_main@GLIBC_2.2.5 at :0 > >> >> 20# _start at :0 > >> >> > >> >> For a non-empty stacktrace_entry, if the function name returned by > >> >> description() is an empty string we now print "<unknown>" for the > >> >> function name. For an empty (e.g. default constructed) > stacktrace_entry > >> >> the entire string representation is now "<unknown>" instead of an > empty > >> >> string. > >> >> > >> >> Instead of printing "<unknown>" for the function name, we could set > that > >> >> string in the stacktrace_entry::_Info object, so that description() > >> >> returns "<unknown>" and then operator<< wouldn't need to handle an > empty > >> >> description() string. However, returning an empty string from that > >> >> function seems simpler for users to detect, rather than having to > parse > >> >> "<unknown>". > >> >> > >> >> We could also choose a different string for an empty > stacktrace_entry, > >> >> maybe "<none>" or "<invalid>", but "<unknown>" seems OK for now. > >> >> > >> >> libstdc++-v3/ChangeLog: > >> >> > >> >> * include/std/stacktrace > >> >> (operator<<(ostream&, const stacktrace_entry&)): Improve > output > >> >> when description() or source_file() returns an empty string, > >> >> or the stacktrace_entry is invalid. > >> >> (operator<<(ostream&, const basic_stacktrace<A>&)): Use > >> >> size_type of the correct specialization. > >> >> --- > >> >> > >> >> v2: Restore fmtflags as pointed out by Nathan. Adjust control flow in > >> >> operator<< as suggested by Tomasz. > >> >> > >> >> Tested x86_64-linux. > >> >> > >> >> libstdc++-v3/include/std/stacktrace | 29 > +++++++++++++++++++++++++---- > >> >> 1 file changed, 25 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/libstdc++-v3/include/std/stacktrace > b/libstdc++-v3/include/std/stacktrace > >> >> index b9e260e19f89..28ffda76328f 100644 > >> >> --- a/libstdc++-v3/include/std/stacktrace > >> >> +++ b/libstdc++-v3/include/std/stacktrace > >> >> @@ -685,11 +685,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> >> { > >> >> string __desc, __file; > >> >> int __line; > >> >> - if (__f._M_get_info(&__desc, &__file, &__line)) > >> >> + if (__f._M_get_info(&__desc, &__file, &__line)) [[likely]] > >> >> { > >> >> - __os.width(4); > >> >> - __os << __desc << " at " << __file << ':' << __line; > >> >> + if (__desc.empty()) [[unlikely]] > >> >> + __os << "<unknown>"; > >> >> + else > >> >> + __os << __desc; > >> >> + __os << string_view(" at "); > >> >> + if (!__file.empty()) [[likely]] > >> >> + return __os << __file << ':' << __line; > >> >> + // else fall through and write native_handle() as the > location. > >> >> } > >> >> + > >> >> + if (__f) [[likely]] > >> >> + { > >> >> + struct _Flag_guard > >> >> + { > >> >> + ios_base& _M_ios; > >> >> + ios_base::fmtflags _M_f; > >> >> + ~_Flag_guard() { _M_ios.setf(_M_f); } > >> >> + }; > >> >> + _Flag_guard _ = { __os, __os.setf(ios::hex, ios::basefield) > }; > >> >> + __os << "[0x" << __f.native_handle() << ']'; > >> >> + } > >> >> + else > >> >> + __os << "<unknown>"; > >> >> return __os; > >> > > >> > I know that I ask for another change ag, but if (!__f) we will not > ever get info for it, so I would prefer: > >> > if (!__f) [[unlikelly] > >> > return __os << "<unknown>"; > >> > > >> > string __desc, __file; > >> > int __line; > >> > if (__f._M_get_info(&__desc, &__file, &__line)) [[likely]] > >> > { > >> > // unchanged > >> > } > >> > > >> > struct _Flag_guard > >> > { > >> > ios_base& _M_ios; > >> > ios_base::fmtflags _M_f; > >> > ~_Flag_guard() { _M_ios.setf(_M_f); } > >> > }; > >> > _Flag_guard _ = { __os, __os.setf(ios::hex, ios::basefield) }; > >> > __os << "[0x" << __f.native_handle() << ']'; > >> > return __os; > >> > > >> > This way it is clear, that we never produce <unknown> at <unknown> > >> > > >> > >> OK, that makes sense. > >> > >> Ignoring the details of the logic in the function for now, the output > >> currently looks like: > >> > >> 15# myfunc(int) at /tmp/bt.cc:48 > >> 16# myfunc(int) at /tmp/bt.cc:48 > >> 17# main at /tmp/bt.cc:61 > >> 18# __libc_start_call_main at [0x7f56578d3574] > >> 19# __libc_start_main@GLIBC_2.2.5 at [0x7f56578d3627] > >> 20# _start at [0x400684] > >> > >> We could output the hex address even when we have a filename, e.g. > >> > >> 15# myfunc(int) at /tmp/bt.cc:48 [0x4008b7] > >> 16# myfunc(int) at /tmp/bt.cc:48 [0x4008b7] > >> 17# main at /tmp/bt.cc:61 [0x40091a] > >> 18# __libc_start_call_main [0x7f0682923574] > >> 19# __libc_start_main@GLIBC_2.2.5 [0x7f0682923627] > >> 20# _start [0x400684] > > > > Maybe we could put the hex address at the front, and then pad it > > 15# [0x4008b7] myfunc(int) at /tmp/bt.cc:48 > > 16# [0x4008b7] myfunc(int) at /tmp/bt.cc:48 > > 17# [0x40091a] main at /tmp/bt.cc:61 > > 18# [0x7f0682923574] __libc_start_call_main > > 19# [0x7f0682923627] __libc_start_main@GLIBC_2.2.5 > > 20# [0x400684] _start > > > > And then maybe pad the address. > > That looks OK for 32-bit targets: > > 15# [0x080487f4] myfunc(int) at /tmp/bt.cc:48 > 16# [0x080487f4] myfunc(int) at /tmp/bt.cc:48 > 17# [0x0804885c] main at /tmp/bt.cc:61 > 18# [0xf7944060] __libc_start_call_main > 19# [0xf7944137] __libc_start_main@GLIBC_2.0 > 20# [0x08048587] _start > > but the padding makes it look very noisy for 64-bit: > > 15# [0x00000000004008c7] myfunc(int) at /tmp/bt.cc:48 > 16# [0x00000000004008c7] myfunc(int) at /tmp/bt.cc:48 > 17# [0x000000000040092a] main at /tmp/bt.cc:61 > 18# [0x00007f8dd65e4574] __libc_start_call_main > 19# [0x00007f8dd65e4627] __libc_start_main@GLIBC_2.2.5 > 20# [0x0000000000400694] _start > That is close what I am used to seeing from gdb and coredumps, thread apply all bt shows: Thread 28 (Thread 0x7ffaa88ff6c0 (LWP 61392) "AsyncSi~lThread"): #0 0x00007ffaba00f0aa in read () from /lib64/libc.so.6 #1 0x00007ffab16fb1af in AsyncSignalControlThreadEntry(void*) () from /usr/lib64/firefox/libxul.so #2 0x000056438d941678 in set_alt_signal_stack_and_start(PthreadCreateParams*) () #3 0x00007ffab9f98724 in start_thread () from /lib64/libc.so.6 #4 0x00007ffaba01c80c in __clone3 () from /lib64/libc.so.6 Thread 27 (Thread 0x7ffaae02e6c0 (LWP 61394) "IPC I/O Child"): #0 0x00007ffaba01cc32 in epoll_wait () from /lib64/libc.so.6 #1 0x00007ffaae088745 in epoll_dispatch.lto_priv () from /lib64/libevent-2.1.so.7 #2 0x00007ffaae080208 in event_base_loop () from /lib64/libevent-2.1.so.7 #3 0x00007ffab10da3ac in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) () from /usr/lib64/firefox/libxul.so #4 0x00007ffab17199bc in MessageLoop::Run() () from /usr/lib64/firefox/libxul.so #5 0x00007ffab171952a in base::Thread::ThreadMain() () from /usr/lib64/firefox/libxul.so #6 0x00007ffab171943b in ThreadFunc(void*) () from /usr/lib64/firefox/libxul.so #7 0x000056438d941678 in set_alt_signal_stack_and_start(PthreadCreateParams*) () #8 0x00007ffab9f98724 in start_thread () from /lib64/libc.so.6 #9 0x00007ffaba01c80c in __clone3 () from /lib64/libc.so.6 > > > That's assuming we pad with zeros. With spaces (and right-aligned) it > looks a bit strange IMHO: > > 15# [0x400917] myfunc(int) at /tmp/bt.cc:48 > 16# [0x400917] myfunc(int) at /tmp/bt.cc:48 > 17# [0x40097a] main at /tmp/bt.cc:61 > 18# [0x7f900a411574] __libc_start_call_main > 19# [0x7f900a411627] __libc_start_main@GLIBC_2.2.5 > 20# [0x4006e4] _start > > > I think I'll push the version that adds the address at the end, unpadded, > and we can revisit it later if we want to. > I am OK with this option also.
