public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
@ 2022-01-02 16:37 soeren
  2022-02-20 10:43 ` Sören Tempel
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: soeren @ 2022-01-02 16:37 UTC (permalink / raw)
  To: gcc-patches

From: Sören Tempel <soeren@soeren-tempel.net>

Both glibc and musl libc declare pt_regs as an incomplete type. This
type has to be completed by inclusion of another header. On Linux, the
asm/ptrace.h header file provides this type definition. Without
including this header file, it is not possible to access the regs member
of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
asm/ptrace.h is not included by other headers and thus the
aforementioned files do not compile without an explicit include of
asm/ptrace.h:

	libgo/runtime/go-signal.c: In function 'getSiginfo':
	libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
	  227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
	      |

See also:

* https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
* https://github.com/kaniini/libucontext/issues/36

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>

ChangeLog:

	* libgo/runtime/go-signal.c: Include asm/ptrace.h for the
	  definition of pt_regs (used by mcontext_t) on PowerPC.
---
 libgo/runtime/go-signal.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index d30d1603adc..fc01e04e4a1 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -10,6 +10,12 @@
 #include <sys/time.h>
 #include <ucontext.h>
 
+// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
+// type. This type must be completed by including asm/ptrace.h.
+#ifdef __PPC__
+#include <asm/ptrace.h>
+#endif
+
 #include "runtime.h"
 
 #ifndef SA_RESTART

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-01-02 16:37 [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC soeren
@ 2022-02-20 10:43 ` Sören Tempel
  2022-02-21 17:25   ` [gofrontend-dev] " Ian Lance Taylor
  2022-02-20 11:01 ` Andreas Schwab
  2022-03-06 18:59 ` [PATCH v2] libgo: Don't use pt_regs member in mcontext_t soeren
  2 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-02-20 10:43 UTC (permalink / raw)
  To: gcc-patches; +Cc: gofrontend-dev

Ping.

Summary: Fix build of libgo on PPC with musl libc and libucontext by
explicitly including the Linux header defining `struct pt_regs` instead of
relying on other libc headers to include it implicitly.

See: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html

If the patch needs to be revised further please let me know. This patch has
been applied at Alpine Linux downstream (which uses musl libc) for a while, I
have not tested it on other systems.

Greetings,
Sören

Sören Tempel <soeren@soeren-tempel.net> wrote:
> Both glibc and musl libc declare pt_regs as an incomplete type. This
> type has to be completed by inclusion of another header. On Linux, the
> asm/ptrace.h header file provides this type definition. Without
> including this header file, it is not possible to access the regs member
> of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
> other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
> asm/ptrace.h is not included by other headers and thus the
> aforementioned files do not compile without an explicit include of
> asm/ptrace.h:
> 
> 	libgo/runtime/go-signal.c: In function 'getSiginfo':
> 	libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> 	  227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> 	      |
> 
> See also:
> 
> * https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
> * https://github.com/kaniini/libucontext/issues/36
> 
> Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> 
> ChangeLog:
> 
> 	* libgo/runtime/go-signal.c: Include asm/ptrace.h for the
> 	  definition of pt_regs (used by mcontext_t) on PowerPC.
> ---
>  libgo/runtime/go-signal.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> index d30d1603adc..fc01e04e4a1 100644
> --- a/libgo/runtime/go-signal.c
> +++ b/libgo/runtime/go-signal.c
> @@ -10,6 +10,12 @@
>  #include <sys/time.h>
>  #include <ucontext.h>
>  
> +// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> +// type. This type must be completed by including asm/ptrace.h.
> +#ifdef __PPC__
> +#include <asm/ptrace.h>
> +#endif
> +
>  #include "runtime.h"
>  
>  #ifndef SA_RESTART

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-01-02 16:37 [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC soeren
  2022-02-20 10:43 ` Sören Tempel
@ 2022-02-20 11:01 ` Andreas Schwab
  2022-03-06 18:59 ` [PATCH v2] libgo: Don't use pt_regs member in mcontext_t soeren
  2 siblings, 0 replies; 25+ messages in thread
From: Andreas Schwab @ 2022-02-20 11:01 UTC (permalink / raw)
  To: soeren--- via Gcc-patches; +Cc: soeren

On Jan 02 2022, soeren--- via Gcc-patches wrote:

> 	libgo/runtime/go-signal.c: In function 'getSiginfo':
> 	libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> 	  227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;

Why does that use .regs instead of .uc_regs?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [gofrontend-dev] Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-02-20 10:43 ` Sören Tempel
@ 2022-02-21 17:25   ` Ian Lance Taylor
  2022-03-06 12:49     ` Sören Tempel
  2022-03-06 15:22     ` Rich Felker
  0 siblings, 2 replies; 25+ messages in thread
From: Ian Lance Taylor @ 2022-02-21 17:25 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

Note for gofrontend-dev: on gcc-patches only Andreas Schwab suggested
using uc_regs instead of regs, which does look correct to me.

Ian

On Mon, Feb 21, 2022 at 8:47 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Ping.
>
> Summary: Fix build of libgo on PPC with musl libc and libucontext by
> explicitly including the Linux header defining `struct pt_regs` instead of
> relying on other libc headers to include it implicitly.
>
> See: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
>
> If the patch needs to be revised further please let me know. This patch has
> been applied at Alpine Linux downstream (which uses musl libc) for a while, I
> have not tested it on other systems.
>
> Greetings,
> Sören
>
> Sören Tempel <soeren@soeren-tempel.net> wrote:
> > Both glibc and musl libc declare pt_regs as an incomplete type. This
> > type has to be completed by inclusion of another header. On Linux, the
> > asm/ptrace.h header file provides this type definition. Without
> > including this header file, it is not possible to access the regs member
> > of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
> > other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
> > asm/ptrace.h is not included by other headers and thus the
> > aforementioned files do not compile without an explicit include of
> > asm/ptrace.h:
> >
> >       libgo/runtime/go-signal.c: In function 'getSiginfo':
> >       libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> >         227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> >             |
> >
> > See also:
> >
> > * https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
> > * https://github.com/kaniini/libucontext/issues/36
> >
> > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> >
> > ChangeLog:
> >
> >       * libgo/runtime/go-signal.c: Include asm/ptrace.h for the
> >         definition of pt_regs (used by mcontext_t) on PowerPC.
> > ---
> >  libgo/runtime/go-signal.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> > index d30d1603adc..fc01e04e4a1 100644
> > --- a/libgo/runtime/go-signal.c
> > +++ b/libgo/runtime/go-signal.c
> > @@ -10,6 +10,12 @@
> >  #include <sys/time.h>
> >  #include <ucontext.h>
> >
> > +// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> > +// type. This type must be completed by including asm/ptrace.h.
> > +#ifdef __PPC__
> > +#include <asm/ptrace.h>
> > +#endif
> > +
> >  #include "runtime.h"
> >
> >  #ifndef SA_RESTART
>
> --
> You received this message because you are subscribed to the Google Groups "gofrontend-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to gofrontend-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/gofrontend-dev/2FICMX0ORZ6O1.3DYXRTDHNGXCN%408pit.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [gofrontend-dev] Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-02-21 17:25   ` [gofrontend-dev] " Ian Lance Taylor
@ 2022-03-06 12:49     ` Sören Tempel
  2022-03-06 15:22     ` Rich Felker
  1 sibling, 0 replies; 25+ messages in thread
From: Sören Tempel @ 2022-03-06 12:49 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: schwab, gcc-patches, gofrontend-dev

What would be the benefit of using .uc_regs instead of .regs? The
current code works entirely fine as it, it just needs an include of the
Linux Kernel header defining the pt_regs type (which my proposed patch
adds).

Furthermore, it seems to me that .uc_regs is only available on powerpc
but not on power64 [1]? musl libc also only defines the .uc_regs member
for powerpc [2] and not for powerpc64 [3] on ucontext_t.

Is there any documentation for .uc_regs on PowerPC?

Sincerely,
Sören

[1]: https://github.com/torvalds/linux/blob/c3617f72036c909e1f6086b5b9e364e0ef90a6da/arch/powerpc/include/uapi/asm/ucontext.h#L24-L27
[2]: https://git.musl-libc.org/cgit/musl/tree/arch/powerpc/bits/signal.h?id=f8bdc3048216f41eaaf655524fa286cfb1184a70#n67
[3]: https://git.musl-libc.org/cgit/musl/tree/arch/powerpc64/bits/signal.h?id=f8bdc3048216f41eaaf655524fa286cfb1184a70#n56

Ian Lance Taylor <iant@golang.org> wrote:
> Note for gofrontend-dev: on gcc-patches only Andreas Schwab suggested
> using uc_regs instead of regs, which does look correct to me.
> 
> Ian
> 
> On Mon, Feb 21, 2022 at 8:47 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
> >
> > Ping.
> >
> > Summary: Fix build of libgo on PPC with musl libc and libucontext by
> > explicitly including the Linux header defining `struct pt_regs` instead of
> > relying on other libc headers to include it implicitly.
> >
> > See: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
> >
> > If the patch needs to be revised further please let me know. This patch has
> > been applied at Alpine Linux downstream (which uses musl libc) for a while, I
> > have not tested it on other systems.
> >
> > Greetings,
> > Sören
> >
> > Sören Tempel <soeren@soeren-tempel.net> wrote:
> > > Both glibc and musl libc declare pt_regs as an incomplete type. This
> > > type has to be completed by inclusion of another header. On Linux, the
> > > asm/ptrace.h header file provides this type definition. Without
> > > including this header file, it is not possible to access the regs member
> > > of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
> > > other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
> > > asm/ptrace.h is not included by other headers and thus the
> > > aforementioned files do not compile without an explicit include of
> > > asm/ptrace.h:
> > >
> > >       libgo/runtime/go-signal.c: In function 'getSiginfo':
> > >       libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> > >         227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> > >             |
> > >
> > > See also:
> > >
> > > * https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
> > > * https://github.com/kaniini/libucontext/issues/36
> > >
> > > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> > >
> > > ChangeLog:
> > >
> > >       * libgo/runtime/go-signal.c: Include asm/ptrace.h for the
> > >         definition of pt_regs (used by mcontext_t) on PowerPC.
> > > ---
> > >  libgo/runtime/go-signal.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> > > index d30d1603adc..fc01e04e4a1 100644
> > > --- a/libgo/runtime/go-signal.c
> > > +++ b/libgo/runtime/go-signal.c
> > > @@ -10,6 +10,12 @@
> > >  #include <sys/time.h>
> > >  #include <ucontext.h>
> > >
> > > +// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> > > +// type. This type must be completed by including asm/ptrace.h.
> > > +#ifdef __PPC__
> > > +#include <asm/ptrace.h>
> > > +#endif
> > > +
> > >  #include "runtime.h"
> > >
> > >  #ifndef SA_RESTART
> >
> > --
> > You received this message because you are subscribed to the Google Groups "gofrontend-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to gofrontend-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/gofrontend-dev/2FICMX0ORZ6O1.3DYXRTDHNGXCN%408pit.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [gofrontend-dev] Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-02-21 17:25   ` [gofrontend-dev] " Ian Lance Taylor
  2022-03-06 12:49     ` Sören Tempel
@ 2022-03-06 15:22     ` Rich Felker
  2022-03-06 16:59       ` Rich Felker
  1 sibling, 1 reply; 25+ messages in thread
From: Rich Felker @ 2022-03-06 15:22 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: Sören Tempel, gcc-patches, gofrontend-dev

On Mon, Feb 21, 2022 at 09:25:43AM -0800, Ian Lance Taylor via Gcc-patches wrote:
> Note for gofrontend-dev: on gcc-patches only Andreas Schwab suggested
> using uc_regs instead of regs, which does look correct to me.

Yes, this is absolutely the correct fix. Having pt_regs appear at all
in code not using ptrace is a serious code smell.

The root of this problem is twofold: (1) ancient Linux (2.0.x?) had a
bad definition of powerpc32 ucontext_t that lacked any mcontext_t,
instead having a regs member pointing to the storage for the register
state (as pt_regs). This was ostensibly done for extensibility
reasons, but was non-POSIX-conforming and broken, and was later fixed.

And (2) glibc's definition of ucontext_t is also non-conforming,
making the uc_mcontext member have type anon-union rather than type
mcontext_t.

musl does not follow this but puts the uc_mcontext member in the place
later kernel ABI assigned to it after the kernel mistake was fixed.

Ideally you would access uc_mcontext.gregs[32] (32==NIP) and be done
with it, but this won't work on glibc because of (2). However musl
also supports the old uc_regs pointer (it's in the reserved namespace
anyway so not a conformance error), making it so uc_regs->gregs[32]
works on either.

Rich



> On Mon, Feb 21, 2022 at 8:47 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
> >
> > Ping.
> >
> > Summary: Fix build of libgo on PPC with musl libc and libucontext by
> > explicitly including the Linux header defining `struct pt_regs` instead of
> > relying on other libc headers to include it implicitly.
> >
> > See: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
> >
> > If the patch needs to be revised further please let me know. This patch has
> > been applied at Alpine Linux downstream (which uses musl libc) for a while, I
> > have not tested it on other systems.
> >
> > Greetings,
> > Sören
> >
> > Sören Tempel <soeren@soeren-tempel.net> wrote:
> > > Both glibc and musl libc declare pt_regs as an incomplete type. This
> > > type has to be completed by inclusion of another header. On Linux, the
> > > asm/ptrace.h header file provides this type definition. Without
> > > including this header file, it is not possible to access the regs member
> > > of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
> > > other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
> > > asm/ptrace.h is not included by other headers and thus the
> > > aforementioned files do not compile without an explicit include of
> > > asm/ptrace.h:
> > >
> > >       libgo/runtime/go-signal.c: In function 'getSiginfo':
> > >       libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> > >         227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> > >             |
> > >
> > > See also:
> > >
> > > * https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
> > > * https://github.com/kaniini/libucontext/issues/36
> > >
> > > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> > >
> > > ChangeLog:
> > >
> > >       * libgo/runtime/go-signal.c: Include asm/ptrace.h for the
> > >         definition of pt_regs (used by mcontext_t) on PowerPC.
> > > ---
> > >  libgo/runtime/go-signal.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> > > index d30d1603adc..fc01e04e4a1 100644
> > > --- a/libgo/runtime/go-signal.c
> > > +++ b/libgo/runtime/go-signal.c
> > > @@ -10,6 +10,12 @@
> > >  #include <sys/time.h>
> > >  #include <ucontext.h>
> > >
> > > +// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> > > +// type. This type must be completed by including asm/ptrace.h.
> > > +#ifdef __PPC__
> > > +#include <asm/ptrace.h>
> > > +#endif
> > > +
> > >  #include "runtime.h"
> > >
> > >  #ifndef SA_RESTART
> >
> > --
> > You received this message because you are subscribed to the Google Groups "gofrontend-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to gofrontend-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/gofrontend-dev/2FICMX0ORZ6O1.3DYXRTDHNGXCN%408pit.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [gofrontend-dev] Re: [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC
  2022-03-06 15:22     ` Rich Felker
@ 2022-03-06 16:59       ` Rich Felker
  0 siblings, 0 replies; 25+ messages in thread
From: Rich Felker @ 2022-03-06 16:59 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gofrontend-dev, gcc-patches, Sören Tempel

On Sun, Mar 06, 2022 at 10:22:56AM -0500, Rich Felker wrote:
> On Mon, Feb 21, 2022 at 09:25:43AM -0800, Ian Lance Taylor via Gcc-patches wrote:
> > Note for gofrontend-dev: on gcc-patches only Andreas Schwab suggested
> > using uc_regs instead of regs, which does look correct to me.
> 
> Yes, this is absolutely the correct fix. Having pt_regs appear at all
> in code not using ptrace is a serious code smell.
> 
> The root of this problem is twofold: (1) ancient Linux (2.0.x?) had a
> bad definition of powerpc32 ucontext_t that lacked any mcontext_t,
> instead having a regs member pointing to the storage for the register
> state (as pt_regs). This was ostensibly done for extensibility
> reasons, but was non-POSIX-conforming and broken, and was later fixed.
> 
> And (2) glibc's definition of ucontext_t is also non-conforming,
> making the uc_mcontext member have type anon-union rather than type
> mcontext_t.
> 
> musl does not follow this but puts the uc_mcontext member in the place
> later kernel ABI assigned to it after the kernel mistake was fixed.
> 
> Ideally you would access uc_mcontext.gregs[32] (32==NIP) and be done
> with it, but this won't work on glibc because of (2). However musl
> also supports the old uc_regs pointer (it's in the reserved namespace
> anyway so not a conformance error), making it so uc_regs->gregs[32]
> works on either.

I mistakenly thought this was ppc32 because I wasn't remembering that
a lesser mess was still present on ppc64. The above applies to ppc32.
On ppc64 it would be uc_mcontext.gp_regs[32].

I'm not sure if the code is intended to also work on ppc32, but even
if that's not supported now, when fixing this it should probably
condition the use of gregs/gp_regs name on __WORDSIZE or whatever so
that, if anyone ever does try to add ppc32 support, they don't get
bogged down in this again and get it wrong again...

Rich



> > On Mon, Feb 21, 2022 at 8:47 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
> > >
> > > Ping.
> > >
> > > Summary: Fix build of libgo on PPC with musl libc and libucontext by
> > > explicitly including the Linux header defining `struct pt_regs` instead of
> > > relying on other libc headers to include it implicitly.
> > >
> > > See: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
> > >
> > > If the patch needs to be revised further please let me know. This patch has
> > > been applied at Alpine Linux downstream (which uses musl libc) for a while, I
> > > have not tested it on other systems.
> > >
> > > Greetings,
> > > Sören
> > >
> > > Sören Tempel <soeren@soeren-tempel.net> wrote:
> > > > Both glibc and musl libc declare pt_regs as an incomplete type. This
> > > > type has to be completed by inclusion of another header. On Linux, the
> > > > asm/ptrace.h header file provides this type definition. Without
> > > > including this header file, it is not possible to access the regs member
> > > > of the mcontext_t struct as done in libgo/runtime/go-signal.c. On glibc,
> > > > other headers (e.g. sys/user.h) include asm/ptrace.h but on musl
> > > > asm/ptrace.h is not included by other headers and thus the
> > > > aforementioned files do not compile without an explicit include of
> > > > asm/ptrace.h:
> > > >
> > > >       libgo/runtime/go-signal.c: In function 'getSiginfo':
> > > >       libgo/runtime/go-signal.c:227:63: error: invalid use of undefined type 'struct pt_regs'
> > > >         227 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> > > >             |
> > > >
> > > > See also:
> > > >
> > > > * https://git.musl-libc.org/cgit/musl/commit/?id=c2518a8efb6507f1b41c3b12e03b06f8f2317a1f
> > > > * https://github.com/kaniini/libucontext/issues/36
> > > >
> > > > Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> > > >
> > > > ChangeLog:
> > > >
> > > >       * libgo/runtime/go-signal.c: Include asm/ptrace.h for the
> > > >         definition of pt_regs (used by mcontext_t) on PowerPC.
> > > > ---
> > > >  libgo/runtime/go-signal.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> > > > index d30d1603adc..fc01e04e4a1 100644
> > > > --- a/libgo/runtime/go-signal.c
> > > > +++ b/libgo/runtime/go-signal.c
> > > > @@ -10,6 +10,12 @@
> > > >  #include <sys/time.h>
> > > >  #include <ucontext.h>
> > > >
> > > > +// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> > > > +// type. This type must be completed by including asm/ptrace.h.
> > > > +#ifdef __PPC__
> > > > +#include <asm/ptrace.h>
> > > > +#endif
> > > > +
> > > >  #include "runtime.h"
> > > >
> > > >  #ifndef SA_RESTART
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "gofrontend-dev" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to gofrontend-dev+unsubscribe@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/gofrontend-dev/2FICMX0ORZ6O1.3DYXRTDHNGXCN%408pit.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2] libgo: Don't use pt_regs member in mcontext_t
  2022-01-02 16:37 [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC soeren
  2022-02-20 10:43 ` Sören Tempel
  2022-02-20 11:01 ` Andreas Schwab
@ 2022-03-06 18:59 ` soeren
  2022-03-06 21:21   ` Rich Felker
  2 siblings, 1 reply; 25+ messages in thread
From: soeren @ 2022-03-06 18:59 UTC (permalink / raw)
  To: gcc-patches; +Cc: iant, gofrontend-dev, schwab, dalias

From: Sören Tempel <soeren+git@soeren-tempel.net>

The .regs member is primarily intended to be used in conjunction with
ptrace. Since this code is not using ptrace, using .regs is a bad idea.
Furthermore, the code currently fails to compile on musl since the
pt_regs type (used by .regs) is in an incomplete type which has to be
completed by inclusion of the asm/ptrace.h Kernel header. Contrary to
glibc, this header is not indirectly included by musl through other
header files.

This patch fixes compilation of this code with musl libc by accessing
the register values via .gp_regs/.gregs (depending on 32-bit or 64-bit
PowerPC) instead of using .regs. For more details, see
https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591261.html

For the offsets in gp_regs refer to the Kernel asm/ptrace.h header.

This patch has been tested on Alpine Linux ppc64le (uses musl libc).

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>

ChangeLog:

	* libgo/runtime/go-signal.c (defined): Use .gp_regs/.gregs
	  to access ppc64/ppc32 registers.
	(dumpregs): Ditto.
---
Changes since v1: Use .gp_regs/.gregs instead of .regs based on
feedback by Rich Felker, thereby avoiding the need to include
asm/ptrace.h for struct pt_regs.

 libgo/runtime/go-signal.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index d30d1603adc..647ad606019 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -224,7 +224,11 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__alpha__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
 #elif defined(__PPC__) && defined(__linux__)
-	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
+#ifdef __PPC64__
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
+#else
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
+#endif
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -338,16 +342,21 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 #elif defined(__PPC__) && defined(__LITTLE_ENDIAN__) && defined(__linux__)
 	  {
 		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
+#ifdef __PPC64__
+		greg_t *gp = &m->gp_regs;
+#else
+		greg_t *gp = &m->gregs;
+#endif
 		int i;
 
 		for (i = 0; i < 32; i++)
-			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
-		runtime_printf("pc  %X\n", m->regs->nip);
-		runtime_printf("msr %X\n", m->regs->msr);
-		runtime_printf("cr  %X\n", m->regs->ccr);
-		runtime_printf("lr  %X\n", m->regs->link);
-		runtime_printf("ctr %X\n", m->regs->ctr);
-		runtime_printf("xer %X\n", m->regs->xer);
+			runtime_printf("r%d %X\n", i, gp[i]);
+		runtime_printf("pc  %X\n", gp[32]);
+		runtime_printf("msr %X\n", gp[33]);
+		runtime_printf("cr  %X\n", gp[38]);
+		runtime_printf("lr  %X\n", gp[36]);
+		runtime_printf("ctr %X\n", gp[35]);
+		runtime_printf("xer %X\n", gp[37]);
 	  }
 #elif defined(__PPC__) && defined(_AIX)
 	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] libgo: Don't use pt_regs member in mcontext_t
  2022-03-06 18:59 ` [PATCH v2] libgo: Don't use pt_regs member in mcontext_t soeren
@ 2022-03-06 21:21   ` Rich Felker
  2022-03-07  7:09     ` [PATCH v3] " soeren
  0 siblings, 1 reply; 25+ messages in thread
From: Rich Felker @ 2022-03-06 21:21 UTC (permalink / raw)
  To: soeren; +Cc: gcc-patches, iant, gofrontend-dev, schwab

On Sun, Mar 06, 2022 at 07:59:24PM +0100, soeren@soeren-tempel.net wrote:
> From: Sören Tempel <soeren+git@soeren-tempel.net>
> 
> The .regs member is primarily intended to be used in conjunction with
> ptrace. Since this code is not using ptrace, using .regs is a bad idea.
> Furthermore, the code currently fails to compile on musl since the
> pt_regs type (used by .regs) is in an incomplete type which has to be
> completed by inclusion of the asm/ptrace.h Kernel header. Contrary to
> glibc, this header is not indirectly included by musl through other
> header files.
> 
> This patch fixes compilation of this code with musl libc by accessing
> the register values via .gp_regs/.gregs (depending on 32-bit or 64-bit
> PowerPC) instead of using .regs. For more details, see
> https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591261.html
> 
> For the offsets in gp_regs refer to the Kernel asm/ptrace.h header.
> 
> This patch has been tested on Alpine Linux ppc64le (uses musl libc).
> 
> Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> 
> ChangeLog:
> 
> 	* libgo/runtime/go-signal.c (defined): Use .gp_regs/.gregs
> 	  to access ppc64/ppc32 registers.
> 	(dumpregs): Ditto.
> ---
> Changes since v1: Use .gp_regs/.gregs instead of .regs based on
> feedback by Rich Felker, thereby avoiding the need to include
> asm/ptrace.h for struct pt_regs.
> 
>  libgo/runtime/go-signal.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> index d30d1603adc..647ad606019 100644
> --- a/libgo/runtime/go-signal.c
> +++ b/libgo/runtime/go-signal.c
> @@ -224,7 +224,11 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
>  #elif defined(__alpha__) && defined(__linux__)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
>  #elif defined(__PPC__) && defined(__linux__)
> -	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> +#ifdef __PPC64__
> +	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
> +#else
> +	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
> +#endif
>  #elif defined(__PPC__) && defined(_AIX)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
>  #elif defined(__aarch64__) && defined(__linux__)
> @@ -338,16 +342,21 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
>  #elif defined(__PPC__) && defined(__LITTLE_ENDIAN__) && defined(__linux__)
>  	  {
>  		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
> +#ifdef __PPC64__
> +		greg_t *gp = &m->gp_regs;
> +#else
> +		greg_t *gp = &m->gregs;
> +#endif

Have you tried compiling this? It looks like it's a constraint
violation because gregset_t has array type and & will produce a
pointer to the array type rather than a pointer to the first element.
You should drop the & to get the pointer to the first element.

Rich

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3] libgo: Don't use pt_regs member in mcontext_t
  2022-03-06 21:21   ` Rich Felker
@ 2022-03-07  7:09     ` soeren
  2022-03-07 22:59       ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: soeren @ 2022-03-07  7:09 UTC (permalink / raw)
  To: gcc-patches; +Cc: iant, gofrontend-dev, schwab, dalias

From: Sören Tempel <soeren@soeren-tempel.net>

The .regs member is primarily intended to be used in conjunction with
ptrace. Since this code is not using ptrace, using .regs is a bad idea.
Furthermore, the code currently fails to compile on musl since the
pt_regs type (used by .regs) is in an incomplete type which has to be
completed by inclusion of the asm/ptrace.h Kernel header. Contrary to
glibc, this header is not indirectly included by musl through other
header files.

This patch fixes compilation of this code with musl libc by accessing
the register values via .gp_regs/.gregs (depending on 32-bit or 64-bit
PowerPC) instead of using .regs. For more details, see
https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591261.html

For the offsets in gp_regs refer to the Kernel asm/ptrace.h header.

This patch has been tested on Alpine Linux ppc64le (uses musl libc).

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>

ChangeLog:

	* libgo/runtime/go-signal.c (defined): Use .gp_regs/.gregs
	  to access ppc64/ppc32 registers.
	(dumpregs): Ditto.
---
Changes since v2: Fixed a minor type mistake which, unfortunately,
did not result in a compilation error.

 libgo/runtime/go-signal.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index d30d1603adc..53fe270e66f 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -224,7 +224,11 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__alpha__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
 #elif defined(__PPC__) && defined(__linux__)
-	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
+#ifdef __PPC64__
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
+#else
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
+#endif
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -338,16 +342,21 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 #elif defined(__PPC__) && defined(__LITTLE_ENDIAN__) && defined(__linux__)
 	  {
 		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
+#ifdef __PPC64__
+		greg_t *gp = m->gp_regs;
+#else
+		greg_t *gp = m->gregs;
+#endif
 		int i;
 
 		for (i = 0; i < 32; i++)
-			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
-		runtime_printf("pc  %X\n", m->regs->nip);
-		runtime_printf("msr %X\n", m->regs->msr);
-		runtime_printf("cr  %X\n", m->regs->ccr);
-		runtime_printf("lr  %X\n", m->regs->link);
-		runtime_printf("ctr %X\n", m->regs->ctr);
-		runtime_printf("xer %X\n", m->regs->xer);
+			runtime_printf("r%d %X\n", i, gp[i]);
+		runtime_printf("pc  %X\n", gp[32]);
+		runtime_printf("msr %X\n", gp[33]);
+		runtime_printf("cr  %X\n", gp[38]);
+		runtime_printf("lr  %X\n", gp[36]);
+		runtime_printf("ctr %X\n", gp[35]);
+		runtime_printf("xer %X\n", gp[37]);
 	  }
 #elif defined(__PPC__) && defined(_AIX)
 	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] libgo: Don't use pt_regs member in mcontext_t
  2022-03-07  7:09     ` [PATCH v3] " soeren
@ 2022-03-07 22:59       ` Ian Lance Taylor
  2022-03-08 13:23         ` Rich Felker
  2022-03-09  7:26         ` Sören Tempel
  0 siblings, 2 replies; 25+ messages in thread
From: Ian Lance Taylor @ 2022-03-07 22:59 UTC (permalink / raw)
  To: soeren; +Cc: gcc-patches, gofrontend-dev, schwab, dalias

On Sun, Mar 6, 2022 at 11:11 PM <soeren@soeren-tempel.net> wrote:
>
> +#ifdef __PPC64__
> +       ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
> +#else
> +       ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
> +#endif

Have you tested this in 32-bit mode?  It does not look correct based
on the glibc definitions.  Looking at glibc it seems that it ought to
be

reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];

Ian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] libgo: Don't use pt_regs member in mcontext_t
  2022-03-07 22:59       ` Ian Lance Taylor
@ 2022-03-08 13:23         ` Rich Felker
  2022-03-09  7:26         ` Sören Tempel
  1 sibling, 0 replies; 25+ messages in thread
From: Rich Felker @ 2022-03-08 13:23 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: soeren, gcc-patches, gofrontend-dev, schwab

On Mon, Mar 07, 2022 at 02:59:02PM -0800, Ian Lance Taylor wrote:
> On Sun, Mar 6, 2022 at 11:11 PM <soeren@soeren-tempel.net> wrote:
> >
> > +#ifdef __PPC64__
> > +       ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
> > +#else
> > +       ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
> > +#endif
> 
> Have you tested this in 32-bit mode?  It does not look correct based
> on the glibc definitions.  Looking at glibc it seems that it ought to
> be
> 
> reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];

Indeed, I think it has to use that conditional on __GLIBC__. I was
thinking the union glibc has was an anon union, but no, it's named
uc_mcontext despite not having type mcontext_t.

Ideally glibc could fix this by doing:

    union {
        union __ctx(uc_regs_ptr) {
            struct __ctx(pt_regs) *__ctx(regs);
            mcontext_t *__ctx(uc_regs);
        } uc_mcontext;
        mcontext_t *__ctx(uc_regs);
    };

so that there would be a common API for accessing it that doesn't
conflict with the properties the standard mandates.

Rich

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] libgo: Don't use pt_regs member in mcontext_t
  2022-03-07 22:59       ` Ian Lance Taylor
  2022-03-08 13:23         ` Rich Felker
@ 2022-03-09  7:26         ` Sören Tempel
  2022-03-09 11:52           ` Rich Felker
  1 sibling, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-03-09  7:26 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gcc-patches, gofrontend-dev, schwab, dalias

Ian Lance Taylor <iant@golang.org> wrote:
> Have you tested this in 32-bit mode?  It does not look correct based
> on the glibc definitions.  Looking at glibc it seems that it ought to
> be

As stated in the commit message, I have only tested this on Alpine Linux
ppc64le (which uses musl libc). Unfortunately, I don't have access to a
32-bit PowerPC machine and hence haven't performed any tests with it.

> reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];

While this should work with glibc, it doesn't work with musl. In order
to support both (musl and glibc) on 32-bit PowerPC, we would have to do
something along the lines of:

	#ifdef __PPC__
	#if defined(__PPC64__)   /* ppc64 glibc & musl */
	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32]
	#elif defined(__GLIBC__) /* ppc32 glibc */
	reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];
	#else                    /* ppc32 musl */
	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
	#endif /* __PPC64__ */
	#endif /* __PPC__ */

In light of these observations, maybe using asm/ptrace.h and .regs (as
proposed in the v1 patch) is the "better" (i.e. more readable) solution
for now? I agree with Rich that using .regs is certainly a "code smell",
but this gigantic ifdef block also looks pretty smelly to me. That being
said, I can also send a v4 which uses this ifdef block.

Greetings,
Sören

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] libgo: Don't use pt_regs member in mcontext_t
  2022-03-09  7:26         ` Sören Tempel
@ 2022-03-09 11:52           ` Rich Felker
  2022-03-11  7:34             ` [PATCH v4] " soeren
  0 siblings, 1 reply; 25+ messages in thread
From: Rich Felker @ 2022-03-09 11:52 UTC (permalink / raw)
  To: Sören Tempel; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev, schwab

On Wed, Mar 09, 2022 at 08:26:11AM +0100, Sören Tempel wrote:
> Ian Lance Taylor <iant@golang.org> wrote:
> > Have you tested this in 32-bit mode?  It does not look correct based
> > on the glibc definitions.  Looking at glibc it seems that it ought to
> > be
> 
> As stated in the commit message, I have only tested this on Alpine Linux
> ppc64le (which uses musl libc). Unfortunately, I don't have access to a
> 32-bit PowerPC machine and hence haven't performed any tests with it.
> 
> > reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];
> 
> While this should work with glibc, it doesn't work with musl. In order
> to support both (musl and glibc) on 32-bit PowerPC, we would have to do
> something along the lines of:
> 
> 	#ifdef __PPC__
> 	#if defined(__PPC64__)   /* ppc64 glibc & musl */
> 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32]
> 	#elif defined(__GLIBC__) /* ppc32 glibc */
> 	reg.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];
> 	#else                    /* ppc32 musl */
> 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
> 	#endif /* __PPC64__ */
> 	#endif /* __PPC__ */
> 
> In light of these observations, maybe using asm/ptrace.h and .regs (as
> proposed in the v1 patch) is the "better" (i.e. more readable) solution
> for now? I agree with Rich that using .regs is certainly a "code smell",
> but this gigantic ifdef block also looks pretty smelly to me. That being
> said, I can also send a v4 which uses this ifdef block.

I still prefer the #ifdef chain here, because it's at least using the
intended API. The problem is just that we can't have a matching API
because the only API glibc offers on ppc32 contradicts the POSIX
requirements.

I'm also not understanding how the .regs approach in the v1 patch was
ever supposed to work with musl anyway. The relevant line was:

	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;

and musl has no uc_mcontext.regs member because uc_mcontext is
mcontext_t, not the glibc ppc32 pointer union thing. The only .regs in
musl's ppc32 signal.h/ucontext.h is in struct sigcontext, not anywhere
in ucontext_t.

Rich

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-03-09 11:52           ` Rich Felker
@ 2022-03-11  7:34             ` soeren
  2022-03-31 16:41               ` Sören Tempel
  0 siblings, 1 reply; 25+ messages in thread
From: soeren @ 2022-03-11  7:34 UTC (permalink / raw)
  To: gcc-patches; +Cc: iant, gofrontend-dev, schwab, dalias

From: Sören Tempel <soeren@soeren-tempel.net>

The .regs member is primarily intended to be used in conjunction with
ptrace. Since this code is not using ptrace, using .regs is a bad idea.
Furthermore, the code currently fails to compile on musl since the
pt_regs type (used by .regs) is in an incomplete type which has to be
completed by inclusion of the asm/ptrace.h Kernel header. Contrary to
glibc, this header is not indirectly included by musl through other
header files.

This patch fixes compilation of this code with musl libc by accessing
the register values via .gp_regs/.gregs (depending on 32-bit or 64-bit
PowerPC) instead of using .regs. For more details, see
https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591261.html

For the offsets in gp_regs refer to the Kernel asm/ptrace.h header.

This patch has been tested on Alpine Linux ppc64le (uses musl libc).

Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>

ChangeLog:

	* libgo/runtime/go-signal.c (defined): Use .gp_regs/.gregs
	  to access ppc64/ppc32 registers.
	(dumpregs): Ditto.
---
Changes since v3: Add special handling for 32-bit PowerPC with glibc,
also avoid use of gregs_t type since glibc does not seem to define
it on PowerPC.

This version of the patch introduces a new macro (PPC_GPREGS) to access
these registers to special case musl/glibc handling in a central place
once instead of duplicating it twice.

 libgo/runtime/go-signal.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index d30d1603adc..3255046260d 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -16,6 +16,21 @@
   #define SA_RESTART 0
 #endif
 
+// The PowerPC API for accessing gregs/gp_regs differs greatly across
+// different libc implementations (musl and glibc).  To workaround that,
+// define the canonical way to access these registers once here.
+//
+// See https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591360.html
+#ifdef __PPC__
+#if defined(__PPC64__)   /* ppc64 glibc & musl */
+#define PPC_GPREGS(MCTX) (MCTX)->gp_regs
+#elif defined(__GLIBC__) /* ppc32 glibc */
+#define PPC_GPREGS(MCTX) (MCTX)->uc_regs->gregs
+#else                    /* ppc32 musl */
+#define PPC_GPREGS(MCTX) (MCTX)->gregs
+#endif /* __PPC64__ */
+#endif /* __PPC__ */
+
 #ifdef USING_SPLIT_STACK
 
 extern void __splitstack_getcontext(void *context[10]);
@@ -224,7 +239,8 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__alpha__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
 #elif defined(__PPC__) && defined(__linux__)
-	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
+	mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
+	ret.sigpc = PPC_GPREGS(m)[32];
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -341,13 +357,13 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		int i;
 
 		for (i = 0; i < 32; i++)
-			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
-		runtime_printf("pc  %X\n", m->regs->nip);
-		runtime_printf("msr %X\n", m->regs->msr);
-		runtime_printf("cr  %X\n", m->regs->ccr);
-		runtime_printf("lr  %X\n", m->regs->link);
-		runtime_printf("ctr %X\n", m->regs->ctr);
-		runtime_printf("xer %X\n", m->regs->xer);
+			runtime_printf("r%d %X\n", i, PPC_GPREGS(m)[i]);
+		runtime_printf("pc  %X\n", PPC_GPREGS(m)[32]);
+		runtime_printf("msr %X\n", PPC_GPREGS(m)[33]);
+		runtime_printf("cr  %X\n", PPC_GPREGS(m)[38]);
+		runtime_printf("lr  %X\n", PPC_GPREGS(m)[36]);
+		runtime_printf("ctr %X\n", PPC_GPREGS(m)[35]);
+		runtime_printf("xer %X\n", PPC_GPREGS(m)[37]);
 	  }
 #elif defined(__PPC__) && defined(_AIX)
 	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-03-11  7:34             ` [PATCH v4] " soeren
@ 2022-03-31 16:41               ` Sören Tempel
  2022-03-31 20:26                 ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-03-31 16:41 UTC (permalink / raw)
  To: gcc-patches; +Cc: gofrontend-dev, iant

Ping.

Would be nice to get this integrated since this one of the changes needed to
make gccgo work with musl libc. Let me know if the patch needs to be revised
further.

Sören Tempel <soeren@soeren-tempel.net> wrote:
> The .regs member is primarily intended to be used in conjunction with
> ptrace. Since this code is not using ptrace, using .regs is a bad idea.
> Furthermore, the code currently fails to compile on musl since the
> pt_regs type (used by .regs) is in an incomplete type which has to be
> completed by inclusion of the asm/ptrace.h Kernel header. Contrary to
> glibc, this header is not indirectly included by musl through other
> header files.
> 
> This patch fixes compilation of this code with musl libc by accessing
> the register values via .gp_regs/.gregs (depending on 32-bit or 64-bit
> PowerPC) instead of using .regs. For more details, see
> https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591261.html
> 
> For the offsets in gp_regs refer to the Kernel asm/ptrace.h header.
> 
> This patch has been tested on Alpine Linux ppc64le (uses musl libc).
> 
> Signed-off-by: Sören Tempel <soeren@soeren-tempel.net>
> 
> ChangeLog:
> 
> 	* libgo/runtime/go-signal.c (defined): Use .gp_regs/.gregs
> 	  to access ppc64/ppc32 registers.
> 	(dumpregs): Ditto.
> ---
> Changes since v3: Add special handling for 32-bit PowerPC with glibc,
> also avoid use of gregs_t type since glibc does not seem to define
> it on PowerPC.
> 
> This version of the patch introduces a new macro (PPC_GPREGS) to access
> these registers to special case musl/glibc handling in a central place
> once instead of duplicating it twice.
> 
>  libgo/runtime/go-signal.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> index d30d1603adc..3255046260d 100644
> --- a/libgo/runtime/go-signal.c
> +++ b/libgo/runtime/go-signal.c
> @@ -16,6 +16,21 @@
>    #define SA_RESTART 0
>  #endif
>  
> +// The PowerPC API for accessing gregs/gp_regs differs greatly across
> +// different libc implementations (musl and glibc).  To workaround that,
> +// define the canonical way to access these registers once here.
> +//
> +// See https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591360.html
> +#ifdef __PPC__
> +#if defined(__PPC64__)   /* ppc64 glibc & musl */
> +#define PPC_GPREGS(MCTX) (MCTX)->gp_regs
> +#elif defined(__GLIBC__) /* ppc32 glibc */
> +#define PPC_GPREGS(MCTX) (MCTX)->uc_regs->gregs
> +#else                    /* ppc32 musl */
> +#define PPC_GPREGS(MCTX) (MCTX)->gregs
> +#endif /* __PPC64__ */
> +#endif /* __PPC__ */
> +
>  #ifdef USING_SPLIT_STACK
>  
>  extern void __splitstack_getcontext(void *context[10]);
> @@ -224,7 +239,8 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
>  #elif defined(__alpha__) && defined(__linux__)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
>  #elif defined(__PPC__) && defined(__linux__)
> -	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> +	mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
> +	ret.sigpc = PPC_GPREGS(m)[32];
>  #elif defined(__PPC__) && defined(_AIX)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
>  #elif defined(__aarch64__) && defined(__linux__)
> @@ -341,13 +357,13 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
>  		int i;
>  
>  		for (i = 0; i < 32; i++)
> -			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
> -		runtime_printf("pc  %X\n", m->regs->nip);
> -		runtime_printf("msr %X\n", m->regs->msr);
> -		runtime_printf("cr  %X\n", m->regs->ccr);
> -		runtime_printf("lr  %X\n", m->regs->link);
> -		runtime_printf("ctr %X\n", m->regs->ctr);
> -		runtime_printf("xer %X\n", m->regs->xer);
> +			runtime_printf("r%d %X\n", i, PPC_GPREGS(m)[i]);
> +		runtime_printf("pc  %X\n", PPC_GPREGS(m)[32]);
> +		runtime_printf("msr %X\n", PPC_GPREGS(m)[33]);
> +		runtime_printf("cr  %X\n", PPC_GPREGS(m)[38]);
> +		runtime_printf("lr  %X\n", PPC_GPREGS(m)[36]);
> +		runtime_printf("ctr %X\n", PPC_GPREGS(m)[35]);
> +		runtime_printf("xer %X\n", PPC_GPREGS(m)[37]);
>  	  }
>  #elif defined(__PPC__) && defined(_AIX)
>  	  {
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-03-31 16:41               ` Sören Tempel
@ 2022-03-31 20:26                 ` Ian Lance Taylor
  2022-04-02  8:21                   ` Sören Tempel
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Lance Taylor @ 2022-03-31 20:26 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Thu, Mar 31, 2022 at 9:41 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Ping.
>
> Would be nice to get this integrated since this one of the changes needed to
> make gccgo work with musl libc. Let me know if the patch needs to be revised
> further.

I went with a simpler solution, more verbose but easier to read.  Now
committed to mainline.  Please let me know if you have any problems
with this.  Thanks.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2280 bytes --]

fad0ecb68c08512ac24852b6d5264cdb9809dc6d
diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index afaccb0e9e6..f93eaf48e28 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-7f33baa09a8172bb2c5f1ca0435d9efe3e194c9b
+45108f37070afb696b069768700e39a269f1fecb
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index 0cb90304730..9c919e1568a 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -231,7 +231,14 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__alpha__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
 #elif defined(__PPC__) && defined(__linux__)
+	// For some reason different libc implementations use
+	// different names.
+#if defined(__PPC64__) || defined(__GLIBC__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
+#else
+	// Assumed to be ppc32 musl.
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
+#endif
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -347,6 +354,7 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
 		int i;
 
+#if defined(__PPC64__) || defined(__GLIBC__)
 		for (i = 0; i < 32; i++)
 			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
 		runtime_printf("pc  %X\n", m->regs->nip);
@@ -355,6 +363,16 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		runtime_printf("lr  %X\n", m->regs->link);
 		runtime_printf("ctr %X\n", m->regs->ctr);
 		runtime_printf("xer %X\n", m->regs->xer);
+#else
+		for (i = 0; i < 32; i++)
+			runtime_printf("r%d %X\n", i, m->gregs[i]);
+		runtime_printf("pc  %X\n", m->gregs[32]);
+		runtime_printf("msr %X\n", m->gregs[33]);
+		runtime_printf("cr  %X\n", m->gregs[38]);
+		runtime_printf("lr  %X\n", m->gregs[36]);
+		runtime_printf("ctr %X\n", m->gregs[35]);
+		runtime_printf("xer %X\n", m->gregs[37]);
+#endif
 	  }
 #elif defined(__PPC__) && defined(_AIX)
 	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-03-31 20:26                 ` Ian Lance Taylor
@ 2022-04-02  8:21                   ` Sören Tempel
  2022-04-03  2:02                     ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-04-02  8:21 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gcc-patches, gofrontend-dev

Hi Ian,

Thanks for committing a first fix! Unfortunately, your changes don't
work on ppc64le musl since you are now still using .regs on ppc64le the
include of asm/ptrace.h (as added in the v1 of my patch) is missing.
Hence, your patch fails to compile on ppc64le musl with the following
error message:

	go-signal.c:230:63: error: invalid use of undefined type 'struct pt_regs'
	  230 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;

If you want to continue using .regs on ppc64le an include of
asm/ptrace.h is needed since both glibc and musl declare `struct
pt_regs` as an incomplete type (with glibc asm/ptrace.h is included
indirectly by other headers used by go-signal.c it seems).

See https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html

Would be nice if this could be fixed :)

Sincerely,
Sören

Ian Lance Taylor <iant@golang.org> wrote:
> On Thu, Mar 31, 2022 at 9:41 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
> >
> > Ping.
> >
> > Would be nice to get this integrated since this one of the changes needed to
> > make gccgo work with musl libc. Let me know if the patch needs to be revised
> > further.
> 
> I went with a simpler solution, more verbose but easier to read.  Now
> committed to mainline.  Please let me know if you have any problems
> with this.  Thanks.
> 
> Ian
> fad0ecb68c08512ac24852b6d5264cdb9809dc6d
> diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
> index afaccb0e9e6..f93eaf48e28 100644
> --- a/gcc/go/gofrontend/MERGE
> +++ b/gcc/go/gofrontend/MERGE
> @@ -1,4 +1,4 @@
> -7f33baa09a8172bb2c5f1ca0435d9efe3e194c9b
> +45108f37070afb696b069768700e39a269f1fecb
>  
>  The first line of this file holds the git revision number of the last
>  merge done from the gofrontend repository.
> diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
> index 0cb90304730..9c919e1568a 100644
> --- a/libgo/runtime/go-signal.c
> +++ b/libgo/runtime/go-signal.c
> @@ -231,7 +231,14 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
>  #elif defined(__alpha__) && defined(__linux__)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
>  #elif defined(__PPC__) && defined(__linux__)
> +	// For some reason different libc implementations use
> +	// different names.
> +#if defined(__PPC64__) || defined(__GLIBC__)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
> +#else
> +	// Assumed to be ppc32 musl.
> +	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
> +#endif
>  #elif defined(__PPC__) && defined(_AIX)
>  	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
>  #elif defined(__aarch64__) && defined(__linux__)
> @@ -347,6 +354,7 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
>  		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
>  		int i;
>  
> +#if defined(__PPC64__) || defined(__GLIBC__)
>  		for (i = 0; i < 32; i++)
>  			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
>  		runtime_printf("pc  %X\n", m->regs->nip);
> @@ -355,6 +363,16 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
>  		runtime_printf("lr  %X\n", m->regs->link);
>  		runtime_printf("ctr %X\n", m->regs->ctr);
>  		runtime_printf("xer %X\n", m->regs->xer);
> +#else
> +		for (i = 0; i < 32; i++)
> +			runtime_printf("r%d %X\n", i, m->gregs[i]);
> +		runtime_printf("pc  %X\n", m->gregs[32]);
> +		runtime_printf("msr %X\n", m->gregs[33]);
> +		runtime_printf("cr  %X\n", m->gregs[38]);
> +		runtime_printf("lr  %X\n", m->gregs[36]);
> +		runtime_printf("ctr %X\n", m->gregs[35]);
> +		runtime_printf("xer %X\n", m->gregs[37]);
> +#endif
>  	  }
>  #elif defined(__PPC__) && defined(_AIX)
>  	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-02  8:21                   ` Sören Tempel
@ 2022-04-03  2:02                     ` Ian Lance Taylor
  2022-04-03  9:28                       ` Sören Tempel
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Lance Taylor @ 2022-04-03  2:02 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

On Sat, Apr 2, 2022 at 1:21 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Thanks for committing a first fix! Unfortunately, your changes don't
> work on ppc64le musl since you are now still using .regs on ppc64le the
> include of asm/ptrace.h (as added in the v1 of my patch) is missing.
> Hence, your patch fails to compile on ppc64le musl with the following
> error message:
>
>         go-signal.c:230:63: error: invalid use of undefined type 'struct pt_regs'
>           230 |         ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
>
> If you want to continue using .regs on ppc64le an include of
> asm/ptrace.h is needed since both glibc and musl declare `struct
> pt_regs` as an incomplete type (with glibc asm/ptrace.h is included
> indirectly by other headers used by go-signal.c it seems).
>
> See https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
>
> Would be nice if this could be fixed :)

Sorry, I guess I misread your patch.

What is the right standalone code for the PPC64 musl case?  Thanks.

Ian

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-03  2:02                     ` Ian Lance Taylor
@ 2022-04-03  9:28                       ` Sören Tempel
  2022-04-11 17:25                         ` Sören Tempel
  0 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-04-03  9:28 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gcc-patches, gofrontend-dev

Ian Lance Taylor <iant@golang.org> wrote:
> Sorry, I guess I misread your patch.

No problem, I think this stuff is hard to get right and understand in
general since it is so poorly documented.

> What is the right standalone code for the PPC64 musl case?  Thanks.

In order to have the current code (i.e. current gofrontend HEAD with
your patch) compile and work with PPC64 musl it would be sufficient to
just include asm/ptrace.h, as proposed in the v1 of my patch [1]:

	// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
	// type. This type must be completed by including asm/ptrace.h.
	#ifdef __PPC__
	#include <asm/ptrace.h>
	#endif

Technically, this should also be needed for using .regs on glibc since
it also declares pt_regs as in incomplete type [5]. As such, adding the
include may be the easiest way to resolve this issue.

However, based on your feedback [2] and feedback by Rich Felker [3]. I
rewrote the go-signal.c PowerPC register handling code to not use .regs
("Having pt_regs appear at all in code not using ptrace is a serious
code smell"). See the v4 of my patch for details [4]. If you don't want
to use .regs on PPC64 musl the right standalone code should be:

	((ucontext_t*)(context))->uc_mcontext.gp_regs;

Unfortunately, this code only works on PPC64 musl and PPC64 glibc not on
PPC32 glibc and PPC32 musl, thus I added a case distinction in the v4 of
my patch [4]. For my personal needs it would be very much sufficient to
just add an include of asm/ptrace.h to go-signal.c to make the current
code (i.e. your patch) also work with PPC64 musl.

Greetings,
Sören

[1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
[2]: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590668.html
[3]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591257.html
[4]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591593.html
[5]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h;hb=6ff3c7714900529b8f5ca64b58d5da9cd5d5b345#l33

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-03  9:28                       ` Sören Tempel
@ 2022-04-11 17:25                         ` Sören Tempel
  2022-04-11 17:35                           ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-04-11 17:25 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gcc-patches, gofrontend-dev

Hi,

Any updates no this?

Sorry I keep bothering you with this but we are quite literally only a
few lines away from having the go-signal.c code compile on PPC64 musl :)

Let me know if you need more information to get this fixed.

Greetings,
Sören

Sören Tempel <soeren@soeren-tempel.net> wrote:
> Ian Lance Taylor <iant@golang.org> wrote:
> > Sorry, I guess I misread your patch.
> 
> No problem, I think this stuff is hard to get right and understand in
> general since it is so poorly documented.
> 
> > What is the right standalone code for the PPC64 musl case?  Thanks.
> 
> In order to have the current code (i.e. current gofrontend HEAD with
> your patch) compile and work with PPC64 musl it would be sufficient to
> just include asm/ptrace.h, as proposed in the v1 of my patch [1]:
> 
> 	// On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> 	// type. This type must be completed by including asm/ptrace.h.
> 	#ifdef __PPC__
> 	#include <asm/ptrace.h>
> 	#endif
> 
> Technically, this should also be needed for using .regs on glibc since
> it also declares pt_regs as in incomplete type [5]. As such, adding the
> include may be the easiest way to resolve this issue.
> 
> However, based on your feedback [2] and feedback by Rich Felker [3]. I
> rewrote the go-signal.c PowerPC register handling code to not use .regs
> ("Having pt_regs appear at all in code not using ptrace is a serious
> code smell"). See the v4 of my patch for details [4]. If you don't want
> to use .regs on PPC64 musl the right standalone code should be:
> 
> 	((ucontext_t*)(context))->uc_mcontext.gp_regs;
> 
> Unfortunately, this code only works on PPC64 musl and PPC64 glibc not on
> PPC32 glibc and PPC32 musl, thus I added a case distinction in the v4 of
> my patch [4]. For my personal needs it would be very much sufficient to
> just add an include of asm/ptrace.h to go-signal.c to make the current
> code (i.e. your patch) also work with PPC64 musl.
> 
> Greetings,
> Sören
> 
> [1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
> [2]: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590668.html
> [3]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591257.html
> [4]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591593.html
> [5]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h;hb=6ff3c7714900529b8f5ca64b58d5da9cd5d5b345#l33

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-11 17:25                         ` Sören Tempel
@ 2022-04-11 17:35                           ` Ian Lance Taylor
  2022-04-11 18:28                             ` Sören Tempel
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Lance Taylor @ 2022-04-11 17:35 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

On Mon, Apr 11, 2022 at 10:26 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Any updates no this?
>
> Sorry I keep bothering you with this but we are quite literally only a
> few lines away from having the go-signal.c code compile on PPC64 musl :)
>
> Let me know if you need more information to get this fixed.

What I was hoping from my earlier question was that you could tell me
the exact lines to write in the current sources that will compile on
MUSL.  Don't include <asm/ptrace.h>, don't refer to earlier patches as
that is what I tried to do earlier but failed, don't add new #define
macros, just add #ifdef and appropriate lines.  Thanks.  If the new
lines also work on glibc using register indexes rather than names,
that would be a bonus.

Ian




> Sören Tempel <soeren@soeren-tempel.net> wrote:
> > Ian Lance Taylor <iant@golang.org> wrote:
> > > Sorry, I guess I misread your patch.
> >
> > No problem, I think this stuff is hard to get right and understand in
> > general since it is so poorly documented.
> >
> > > What is the right standalone code for the PPC64 musl case?  Thanks.
> >
> > In order to have the current code (i.e. current gofrontend HEAD with
> > your patch) compile and work with PPC64 musl it would be sufficient to
> > just include asm/ptrace.h, as proposed in the v1 of my patch [1]:
> >
> >       // On PowerPC, ucontext.h uses a pt_regs struct as an incomplete
> >       // type. This type must be completed by including asm/ptrace.h.
> >       #ifdef __PPC__
> >       #include <asm/ptrace.h>
> >       #endif
> >
> > Technically, this should also be needed for using .regs on glibc since
> > it also declares pt_regs as in incomplete type [5]. As such, adding the
> > include may be the easiest way to resolve this issue.
> >
> > However, based on your feedback [2] and feedback by Rich Felker [3]. I
> > rewrote the go-signal.c PowerPC register handling code to not use .regs
> > ("Having pt_regs appear at all in code not using ptrace is a serious
> > code smell"). See the v4 of my patch for details [4]. If you don't want
> > to use .regs on PPC64 musl the right standalone code should be:
> >
> >       ((ucontext_t*)(context))->uc_mcontext.gp_regs;
> >
> > Unfortunately, this code only works on PPC64 musl and PPC64 glibc not on
> > PPC32 glibc and PPC32 musl, thus I added a case distinction in the v4 of
> > my patch [4]. For my personal needs it would be very much sufficient to
> > just add an include of asm/ptrace.h to go-signal.c to make the current
> > code (i.e. your patch) also work with PPC64 musl.
> >
> > Greetings,
> > Sören
> >
> > [1]: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587520.html
> > [2]: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590668.html
> > [3]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591257.html
> > [4]: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591593.html
> > [5]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h;hb=6ff3c7714900529b8f5ca64b58d5da9cd5d5b345#l33

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-11 17:35                           ` Ian Lance Taylor
@ 2022-04-11 18:28                             ` Sören Tempel
  2022-04-14 22:15                               ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: Sören Tempel @ 2022-04-11 18:28 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: gcc-patches, gofrontend-dev

Ian Lance Taylor <iant@golang.org> wrote:
> What I was hoping from my earlier question was that you could tell me
> the exact lines to write in the current sources that will compile on
> MUSL. Don't include <asm/ptrace.h>, don't refer to earlier patches as
> that is what I tried to do earlier but failed, don't add new #define
> macros, just add #ifdef and appropriate lines.  Thanks.  If the new
> lines also work on glibc using register indexes rather than names,
> that would be a bonus.

Sorry, may bad. Here you go:

diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index 9c919e15..454da75e 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -233,8 +233,11 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__PPC__) && defined(__linux__)
 	// For some reason different libc implementations use
 	// different names.
-#if defined(__PPC64__) || defined(__GLIBC__)
+#if defined(__GLIBC__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
+#elif defined(__PPC64__)
+	// Assumed to be ppc64 musl.
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
 #else
 	// Assumed to be ppc32 musl.
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
@@ -354,7 +357,7 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
 		int i;
 
-#if defined(__PPC64__) || defined(__GLIBC__)
+#if defined(__GLIBC__)
 		for (i = 0; i < 32; i++)
 			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
 		runtime_printf("pc  %X\n", m->regs->nip);
@@ -363,6 +366,15 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		runtime_printf("lr  %X\n", m->regs->link);
 		runtime_printf("ctr %X\n", m->regs->ctr);
 		runtime_printf("xer %X\n", m->regs->xer);
+#elif defined(__PPC64__)
+		for (i = 0; i < 32; i++)
+			runtime_printf("r%d %X\n", i, m->gp_regs[i]);
+		runtime_printf("pc  %X\n", m->gp_regs[32]);
+		runtime_printf("msr %X\n", m->gp_regs[33]);
+		runtime_printf("cr  %X\n", m->gp_regs[38]);
+		runtime_printf("lr  %X\n", m->gp_regs[36]);
+		runtime_printf("ctr %X\n", m->gp_regs[35]);
+		runtime_printf("xer %X\n", m->gp_regs[37]);
 #else
 		for (i = 0; i < 32; i++)
 			runtime_printf("r%d %X\n", i, m->gregs[i]);


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-11 18:28                             ` Sören Tempel
@ 2022-04-14 22:15                               ` Ian Lance Taylor
  2022-04-21  0:50                                 ` Ian Lance Taylor
  0 siblings, 1 reply; 25+ messages in thread
From: Ian Lance Taylor @ 2022-04-14 22:15 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Mon, Apr 11, 2022 at 11:28 AM Sören Tempel <soeren@soeren-tempel.net> wrote:
>
> Ian Lance Taylor <iant@golang.org> wrote:
> > What I was hoping from my earlier question was that you could tell me
> > the exact lines to write in the current sources that will compile on
> > MUSL. Don't include <asm/ptrace.h>, don't refer to earlier patches as
> > that is what I tried to do earlier but failed, don't add new #define
> > macros, just add #ifdef and appropriate lines.  Thanks.  If the new
> > lines also work on glibc using register indexes rather than names,
> > that would be a bonus.
>
> Sorry, may bad. Here you go:

Thanks!  I tested a version of that code with glibc, and it works
there too, so I've committed this patch after testing on
powerpc-linux-gnu and x86_64-linux-gnu.  Please let me know about any
problems.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2477 bytes --]

5c66a1182acceebf9fbcf02039d85a53c9c18bf1
diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index f93eaf48e28..75ee2e3aaca 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-45108f37070afb696b069768700e39a269f1fecb
+323ab0e6fab89978bdbd83dca9c2ad9c5dcd690f
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index 9c919e1568a..2caddd068d6 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -230,15 +230,10 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[REG_EIP];
 #elif defined(__alpha__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.sc_pc;
+#elif defined(__PPC64__) && defined(__linux__)
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
 #elif defined(__PPC__) && defined(__linux__)
-	// For some reason different libc implementations use
-	// different names.
-#if defined(__PPC64__) || defined(__GLIBC__)
-	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.regs->nip;
-#else
-	// Assumed to be ppc32 musl.
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
-#endif
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -354,15 +349,15 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
 		int i;
 
-#if defined(__PPC64__) || defined(__GLIBC__)
+#if defined(__PPC64__)
 		for (i = 0; i < 32; i++)
-			runtime_printf("r%d %X\n", i, m->regs->gpr[i]);
-		runtime_printf("pc  %X\n", m->regs->nip);
-		runtime_printf("msr %X\n", m->regs->msr);
-		runtime_printf("cr  %X\n", m->regs->ccr);
-		runtime_printf("lr  %X\n", m->regs->link);
-		runtime_printf("ctr %X\n", m->regs->ctr);
-		runtime_printf("xer %X\n", m->regs->xer);
+			runtime_printf("r%d %X\n", i, m->gp_regs[i]);
+		runtime_printf("pc  %X\n", m->gp_regs[32]);
+		runtime_printf("msr %X\n", m->gp_regs[33]);
+		runtime_printf("cr  %X\n", m->gp_regs[38]);
+		runtime_printf("lr  %X\n", m->gp_regs[36]);
+		runtime_printf("ctr %X\n", m->gp_regs[35]);
+		runtime_printf("xer %X\n", m->gp_regs[37]);
 #else
 		for (i = 0; i < 32; i++)
 			runtime_printf("r%d %X\n", i, m->gregs[i]);

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t
  2022-04-14 22:15                               ` Ian Lance Taylor
@ 2022-04-21  0:50                                 ` Ian Lance Taylor
  0 siblings, 0 replies; 25+ messages in thread
From: Ian Lance Taylor @ 2022-04-21  0:50 UTC (permalink / raw)
  To: Sören Tempel; +Cc: gcc-patches, gofrontend-dev

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

On Thu, Apr 14, 2022 at 3:15 PM Ian Lance Taylor <iant@golang.org> wrote:
>
> Thanks!  I tested a version of that code with glibc, and it works
> there too, so I've committed this patch after testing on
> powerpc-linux-gnu and x86_64-linux-gnu.  Please let me know about any
> problems.

Well, that patch broke PPC 32-bit, as reported in PR 105315, so I've
committed this one.  Tested on powerpc-linux-gnu, powerpc64-linux-gnu,
powerpc64le-linux-gnu, all with glibc.  I hope that it doesn't break
musl again.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2968 bytes --]

8e14028002a661be19619ee8df081b713a8ec4a5
diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE
index 63238715bd0..ef20a0aafd6 100644
--- a/gcc/go/gofrontend/MERGE
+++ b/gcc/go/gofrontend/MERGE
@@ -1,4 +1,4 @@
-99ca6be406a5781be078ff23f45a72b4c84b16e3
+70ca85f08edf63f46c87d540fa99c45e2903edc2
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
diff --git a/libgo/runtime/go-signal.c b/libgo/runtime/go-signal.c
index 2caddd068d6..528d9b6d9fe 100644
--- a/libgo/runtime/go-signal.c
+++ b/libgo/runtime/go-signal.c
@@ -233,7 +233,11 @@ getSiginfo(siginfo_t *info, void *context __attribute__((unused)))
 #elif defined(__PPC64__) && defined(__linux__)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gp_regs[32];
 #elif defined(__PPC__) && defined(__linux__)
+# if defined(__GLIBC__)
+	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.uc_regs->gregs[32];
+# else
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.gregs[32];
+# endif
 #elif defined(__PPC__) && defined(_AIX)
 	ret.sigpc = ((ucontext_t*)(context))->uc_mcontext.jmp_context.iar;
 #elif defined(__aarch64__) && defined(__linux__)
@@ -344,12 +348,13 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		runtime_printf("sp  %X\n", m->sc_regs[30]);
 		runtime_printf("pc  %X\n", m->sc_pc);
 	  }
-#elif defined(__PPC__) && defined(__LITTLE_ENDIAN__) && defined(__linux__)
+#elif defined(__PPC__) && defined(__linux__)
 	  {
-		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
 		int i;
 
-#if defined(__PPC64__)
+# if defined(__PPC64__)
+		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
+
 		for (i = 0; i < 32; i++)
 			runtime_printf("r%d %X\n", i, m->gp_regs[i]);
 		runtime_printf("pc  %X\n", m->gp_regs[32]);
@@ -358,16 +363,22 @@ dumpregs(siginfo_t *info __attribute__((unused)), void *context __attribute__((u
 		runtime_printf("lr  %X\n", m->gp_regs[36]);
 		runtime_printf("ctr %X\n", m->gp_regs[35]);
 		runtime_printf("xer %X\n", m->gp_regs[37]);
-#else
+# else
+#  if defined(__GLIBC__)
+		mcontext_t *m = ((ucontext_t*)(context))->uc_mcontext.uc_regs;
+#  else
+		mcontext_t *m = &((ucontext_t*)(context))->uc_mcontext;
+#  endif
+
 		for (i = 0; i < 32; i++)
-			runtime_printf("r%d %X\n", i, m->gregs[i]);
-		runtime_printf("pc  %X\n", m->gregs[32]);
-		runtime_printf("msr %X\n", m->gregs[33]);
-		runtime_printf("cr  %X\n", m->gregs[38]);
-		runtime_printf("lr  %X\n", m->gregs[36]);
-		runtime_printf("ctr %X\n", m->gregs[35]);
-		runtime_printf("xer %X\n", m->gregs[37]);
-#endif
+			runtime_printf("r%d %x\n", i, m->gregs[i]);
+		runtime_printf("pc  %x\n", m->gregs[32]);
+		runtime_printf("msr %x\n", m->gregs[33]);
+		runtime_printf("cr  %x\n", m->gregs[38]);
+		runtime_printf("lr  %x\n", m->gregs[36]);
+		runtime_printf("ctr %x\n", m->gregs[35]);
+		runtime_printf("xer %x\n", m->gregs[37]);
+# endif
 	  }
 #elif defined(__PPC__) && defined(_AIX)
 	  {

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2022-04-21  0:50 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-02 16:37 [PATCH] libgo: include asm/ptrace.h for pt_regs definition on PowerPC soeren
2022-02-20 10:43 ` Sören Tempel
2022-02-21 17:25   ` [gofrontend-dev] " Ian Lance Taylor
2022-03-06 12:49     ` Sören Tempel
2022-03-06 15:22     ` Rich Felker
2022-03-06 16:59       ` Rich Felker
2022-02-20 11:01 ` Andreas Schwab
2022-03-06 18:59 ` [PATCH v2] libgo: Don't use pt_regs member in mcontext_t soeren
2022-03-06 21:21   ` Rich Felker
2022-03-07  7:09     ` [PATCH v3] " soeren
2022-03-07 22:59       ` Ian Lance Taylor
2022-03-08 13:23         ` Rich Felker
2022-03-09  7:26         ` Sören Tempel
2022-03-09 11:52           ` Rich Felker
2022-03-11  7:34             ` [PATCH v4] " soeren
2022-03-31 16:41               ` Sören Tempel
2022-03-31 20:26                 ` Ian Lance Taylor
2022-04-02  8:21                   ` Sören Tempel
2022-04-03  2:02                     ` Ian Lance Taylor
2022-04-03  9:28                       ` Sören Tempel
2022-04-11 17:25                         ` Sören Tempel
2022-04-11 17:35                           ` Ian Lance Taylor
2022-04-11 18:28                             ` Sören Tempel
2022-04-14 22:15                               ` Ian Lance Taylor
2022-04-21  0:50                                 ` Ian Lance Taylor

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).