Message ID | 20231116172021.1344351-1-jwakely@redhat.com |
---|---|
State | New |
Headers | show |
Series | [committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564] | expand |
> From: Jonathan Wakely <jwakely@redhat.com> > Date: Thu, 16 Nov 2023 17:20:09 +0000 > PR libstdc++/112564 > * include/std/stacktrace (formatter::format): Format according > to format-spec. > * include/std/thread (formatter::format): Use _Align_right as > default. > * testsuite/19_diagnostics/stacktrace/output.cc: Check > fill-and-align handling. Change compile test to run. > * testsuite/30_threads/thread/id/output.cc: Check fill-and-align > handling. You already know this, so JFTR: this introduced a regression for some targets, logged as PR112630. Was this change deliberate: > --- a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc > +++ b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc > @@ -1,4 +1,5 @@ > -// { dg-do compile { target c++23 } } > +// { dg-options "-lstdc++exp" } > +// { dg-do run { target c++23 } } > // { dg-require-effective-target stacktrace } > // { dg-add-options no_pch } i.e. changing from dg-compile to dg-run? I'm guessing so. Though the changelog entry and post isn't explicit, the use of VERIFY is rather clear and most tests in 19_diagnostics/stacktrace are dg-run. If so, can the "dg-run-ness" of the test please move to a separate test and let 19_diagnostics/stacktrace/output.cc be just dg-compile? This particular test may not warrant the consideration, but more so a pattern to follow for other tests. brgds, H-P PS. Sorry, I have no idea why regarding the underlying multi-target problem
On Mon, 20 Nov 2023 at 02:13, Hans-Peter Nilsson <hp@axis.com> wrote: > > > From: Jonathan Wakely <jwakely@redhat.com> > > Date: Thu, 16 Nov 2023 17:20:09 +0000 > > > PR libstdc++/112564 > > * include/std/stacktrace (formatter::format): Format according > > to format-spec. > > * include/std/thread (formatter::format): Use _Align_right as > > default. > > * testsuite/19_diagnostics/stacktrace/output.cc: Check > > fill-and-align handling. Change compile test to run. > > * testsuite/30_threads/thread/id/output.cc: Check fill-and-align > > handling. > > You already know this, so JFTR: this introduced a regression > for some targets, logged as PR112630. > > Was this change deliberate: > > > --- a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc > > +++ b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc > > @@ -1,4 +1,5 @@ > > -// { dg-do compile { target c++23 } } > > +// { dg-options "-lstdc++exp" } > > +// { dg-do run { target c++23 } } > > // { dg-require-effective-target stacktrace } > > // { dg-add-options no_pch } > > i.e. changing from dg-compile to dg-run? Yes, it was always supposed to be a run test, the old dg-do was a typo that I only noticed when fixing the formatting bug (PR 112564). > I'm guessing so. Though the changelog entry and post isn't > explicit, the use of VERIFY is rather clear and most tests > in 19_diagnostics/stacktrace are dg-run. The changelog entry does say "Change compile test to run." > > If so, can the "dg-run-ness" of the test please move to a > separate test and let 19_diagnostics/stacktrace/output.cc be > just dg-compile? This particular test may not warrant the > consideration, but more so a pattern to follow for other > tests. I don't see any point in doing that here, being able to compile code doing I/O on stacktraces but not run it isn't useful. It needs to be a run test. We do it elsewhere if it's meaningful, e.g. several testsuite/std/format/* tests, and the ones I just added in r14-5562-g568eb2d25c8f79 are all 'compile' only. > > brgds, H-P > PS. Sorry, I have no idea why regarding the underlying multi-target problem I have some vague speculation in PR 112541.
> From: Jonathan Wakely <jwakely.gcc@gmail.com> > Date: Mon, 20 Nov 2023 11:55:22 +0000 > The changelog entry does say "Change compile test to run." Wow, it's right there. The doh:est of doh:s on me. Sorry for wasting your time on that. > > PS. Sorry, I have no idea why regarding the underlying multi-target problem > > I have some vague speculation in PR 112541. From comment #1: "It appears that __stacktrace_impl::_S_current returns an empty sequence of frames. It's possible that all the lambda frames are inlined, or skip+2 in stacktrace.cc causes us to skip real frames that we should keep, or maybe libbacktrace just doesn't work on this target." All more or less likely, with libbacktrace not working for three targets maybe less likely. Or more. Anyway, if I haven't found time to look at this myself within...say two months, I think I'll xfail it for cris-elf. brgds, H-P
On Mon, 20 Nov 2023 at 16:18, Hans-Peter Nilsson <hp@axis.com> wrote: > > > From: Jonathan Wakely <jwakely.gcc@gmail.com> > > Date: Mon, 20 Nov 2023 11:55:22 +0000 > > > The changelog entry does say "Change compile test to run." > > Wow, it's right there. The doh:est of doh:s on me. Sorry > for wasting your time on that. Heh, no real time wasted, it was useful to mark PR 112541 as affecting more than just armv8. > > > > PS. Sorry, I have no idea why regarding the underlying multi-target problem > > > > I have some vague speculation in PR 112541. > > From comment #1: > "It appears that __stacktrace_impl::_S_current returns an > empty sequence of frames. > > It's possible that all the lambda frames are inlined, or > skip+2 in stacktrace.cc causes us to skip real frames that > we should keep, or maybe libbacktrace just doesn't work on > this target." > > All more or less likely, with libbacktrace not working for > three targets maybe less likely. Or more. > > Anyway, if I haven't found time to look at this myself > within...say two months, I think I'll xfail it for cris-elf. I hope to have some time to look into the libbacktrace behaviour myself. I wasn't seeing it on any of the targets I can test myself, but I recently did a bootstrap+regtest on sparc-sub-solaris-2.11 and it fails there too. So maybe I'll be able to debug it soon.
diff --git a/libstdc++-v3/include/std/stacktrace b/libstdc++-v3/include/std/stacktrace index 9d5f6396aed..f570745fe51 100644 --- a/libstdc++-v3/include/std/stacktrace +++ b/libstdc++-v3/include/std/stacktrace @@ -740,7 +740,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { std::ostringstream __os; __os << __x; - return __format::__write(__fc.out(), __os.view()); + auto __str = __os.view(); + return __format::__write_padded_as_spec(__str, __str.size(), + __fc, _M_spec); } private: diff --git a/libstdc++-v3/include/std/thread b/libstdc++-v3/include/std/thread index 39042d7cdf5..ee3b8b1fcb0 100644 --- a/libstdc++-v3/include/std/thread +++ b/libstdc++-v3/include/std/thread @@ -335,7 +335,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __os << __id; auto __str = __os.view(); return __format::__write_padded_as_spec(__str, __str.size(), - __fc, _M_spec); + __fc, _M_spec, + __format::_Align_right); } private: diff --git a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc index 4960ccb85b8..67f1e0cebaf 100644 --- a/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc +++ b/libstdc++-v3/testsuite/19_diagnostics/stacktrace/output.cc @@ -1,4 +1,5 @@ -// { dg-do compile { target c++23 } } +// { dg-options "-lstdc++exp" } +// { dg-do run { target c++23 } } // { dg-require-effective-target stacktrace } // { dg-add-options no_pch } @@ -17,7 +18,8 @@ test_to_string() { auto trace = std::stacktrace::current(); std::string s1 = std::to_string(trace.at(0)); - VERIFY( s1.contains("test_to_string():15") ); + VERIFY( s1.contains("test_to_string()") ); + VERIFY( s1.contains("output.cc:19") ); std::string s2 = std::to_string(trace); VERIFY( s2.contains(s1) ); } @@ -47,7 +49,17 @@ test_format() std::stacktrace_entry entry = trace.at(0); std::string str = std::to_string(entry); VERIFY( std::format("{}", entry) == str ); - VERIFY( std::format("{0:!<{1}}", entry, str.size() + 3) == (str + "!!!") ); + auto len = str.size(); + // with width + VERIFY( std::format("{0:{1}}", entry, len + 1) == (str + " ") ); + // with align + width + VERIFY( std::format("{0:<{1}}", entry, len + 2) == (str + " ") ); + VERIFY( std::format("{0:^{1}}", entry, len + 3) == (" " + str + " ") ); + VERIFY( std::format("{0:>{1}}", entry, len + 4) == (" " + str) ); + // with fill-and-align + width + VERIFY( std::format("{0:!<{1}}", entry, len + 2) == (str + "!!") ); + VERIFY( std::format("{0:!^{1}}", entry, len + 3) == ("!" + str + "!!") ); + VERIFY( std::format("{0:!>{1}}", entry, len + 4) == ("!!!!" + str) ); } int main() diff --git a/libstdc++-v3/testsuite/30_threads/thread/id/output.cc b/libstdc++-v3/testsuite/30_threads/thread/id/output.cc index 08d8c899fda..3c167202b02 100644 --- a/libstdc++-v3/testsuite/30_threads/thread/id/output.cc +++ b/libstdc++-v3/testsuite/30_threads/thread/id/output.cc @@ -80,8 +80,18 @@ test02() auto len = s1.size(); out.str(""); - auto s2 = std::format("{0:x^{1}}", j, len + 4); - VERIFY( s2 == ("xx" + s1 + "xx") ); + std::string s2; + // with width + s2 = std::format("{0:{1}}", j, len + 2); + VERIFY( s2 == (" " + s1) ); + // with align + width + s2 = std::format("{0:>{1}}", j, len + 2); + VERIFY( s2 == (" " + s1) ); + s2 = std::format("{0:<{1}}", j, len + 2); + VERIFY( s2 == (s1 + " ") ); + // with fill-and-align + width + s2 = std::format("{0:x^{1}}", j, len + 5); + VERIFY( s2 == ("xx" + s1 + "xxx") ); #ifdef _GLIBCXX_USE_WCHAR_T static_assert( std::is_default_constructible_v<std::formatter<std::thread::id, wchar_t>> );