Message ID | 20220311073406.26722-1-soeren@soeren-tempel.net |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DCA6A385780C for <patchwork@sourceware.org>; Fri, 11 Mar 2022 07:34:57 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DCA6A385780C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1646984097; bh=ibIvNbqsrW7qJfHCGVvezNrR7uY93qg+smREgIOWGRE=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=f1v0F1YJEWWyDya1/r2iEo/tdS31QVq1j9MMf56ET8DDnqCd2JEqgYu35SqM1moYG i8PEv+OnqxJ7y7h0B4LjTDnQ06OtcGxUPitLnMbNmnlupNNookPMvBJY+8vBnlrnH8 6vySLMAdknMv+OvshBIC9h7iXuN50V/G3cc7t7Cs= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from magnesium.8pit.net (magnesium.8pit.net [45.76.88.171]) by sourceware.org (Postfix) with ESMTP id 423A13857C41; Fri, 11 Mar 2022 07:34:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 423A13857C41 Received: from localhost (ip5f5ae040.dynamic.kabel-deutschland.de [95.90.224.64]) by magnesium.8pit.net (OpenSMTPD) with ESMTPSA id e28c8df8 (TLSv1.3:AEAD-AES256-GCM-SHA384:256:YES); Fri, 11 Mar 2022 08:34:25 +0100 (CET) To: gcc-patches@gcc.gnu.org Subject: [PATCH v4] libgo: Don't use pt_regs member in mcontext_t Date: Fri, 11 Mar 2022 08:34:06 +0100 Message-Id: <20220311073406.26722-1-soeren@soeren-tempel.net> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20220309115236.GU7074@brightrain.aerifal.cx> References: <20220309115236.GU7074@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: soeren--- via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: soeren@soeren-tempel.net Cc: dalias@libc.org, schwab@linux-m68k.org, iant@golang.org, gofrontend-dev@googlegroups.com Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[v4] libgo: Don't use pt_regs member in mcontext_t
|
|
Commit Message
Li, Pan2 via Gcc-patches
March 11, 2022, 7:34 a.m. UTC
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(-)
Comments
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) > { >
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) {
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) > {
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
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
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
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
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]);
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 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]);
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 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) {
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) {