[committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[Jonathan Wakely]

Tested x86_64-linux. Pushed to trunk.

-- >8 --

The formatter for std::thread::id should default to right-align, and the
formatter for std::stacktrace_entry should not just ignore the
fill-and-align and width from the format-spec!

libstdc++-v3/ChangeLog:

	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.
---
 libstdc++-v3/include/std/stacktrace            |  4 +++-
 libstdc++-v3/include/std/thread                |  3 ++-
 .../19_diagnostics/stacktrace/output.cc        | 18 +++++++++++++++---
 .../testsuite/30_threads/thread/id/output.cc   | 14 ++++++++++++--
 4 files changed, 32 insertions(+), 7 deletions(-)

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>> );
-- 
2.41.0

Re: [committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[Hans-Peter Nilsson]

> 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

Re: [committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[Jonathan Wakely]

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.

Re: [committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[Hans-Peter Nilsson]

> 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

Re: [committed] libstdc++: Fix aligned formatting of stacktrace_entry and thread::id [PR112564]

[Jonathan Wakely]

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.