[v3] libgo: Don't use pt_regs member in mcontext_t
Commit Message
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(-)
Comments
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
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
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
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
@@ -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)
{