On Thu, Oct 16, 2025 at 2:22 PM 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. > I am fine either way, but I think always adding an address is an improvement. > > >> >> So only write the " at " string when we have a filename, but always >> show the hex address. >> >> This could be useful if the file:line is the location of a macro >> definition and several functions have the same file:line, but would >> still have a different address. >> >> So something like: >> >> if (!__f) [[unlikely]] >> return __os << "<unknown>"; >> >> string __desc, __file; >> int __line; >> if (__f._M_get_info(&__desc, &__file, &__line)) [[likely]] >> { >> if (__desc.empty()) [[unlikely]] >> __os << "<unknown>"; >> else >> __os << __desc; >> if (!__file.empty()) [[likely]] >> __os << " at " << __file << ':' << __line; >> } >> >> 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; >> >> >
