From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9753 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH 2/2] add powerpc64 port Date: Sun, 27 Mar 2016 19:37:09 -0400 Message-ID: <20160327233709.GE21636@brightrain.aerifal.cx> References: <1459113619-24090-1-git-send-email-koorogi@koorogi.info> <1459113619-24090-3-git-send-email-koorogi@koorogi.info> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1459121846 17613 80.91.229.3 (27 Mar 2016 23:37:26 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 27 Mar 2016 23:37:26 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9766-gllmg-musl=m.gmane.org@lists.openwall.com Mon Mar 28 01:37:26 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1akKFI-0004fT-Qu for gllmg-musl@m.gmane.org; Mon, 28 Mar 2016 01:37:25 +0200 Original-Received: (qmail 9436 invoked by uid 550); 27 Mar 2016 23:37:22 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 9418 invoked from network); 27 Mar 2016 23:37:21 -0000 Content-Disposition: inline In-Reply-To: <1459113619-24090-3-git-send-email-koorogi@koorogi.info> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9753 Archived-At: On Sun, Mar 27, 2016 at 04:20:19PM -0500, Bobby Bingham wrote: > diff --git a/arch/powerpc64/bits/endian.h b/arch/powerpc64/bits/endian.h > new file mode 100644 > index 0000000..2016cb2 > --- /dev/null > +++ b/arch/powerpc64/bits/endian.h > @@ -0,0 +1,5 @@ > +#if __BIG_ENDIAN__ > +#define __BYTE_ORDER __BIG_ENDIAN > +#else > +#define __BYTE_ORDER __LITTLE_ENDIAN > +#endif This should probably use whatever the official psABI macro for PPC BE is rather than __BIG_ENDIAN__ (which I think is a confusingly-named GCC thing; it's not clear whether it means "is big endian" or "value that indicates big endian"). > diff --git a/arch/powerpc64/bits/sem.h b/arch/powerpc64/bits/sem.h > new file mode 100644 > index 0000000..5f04979 > --- /dev/null > +++ b/arch/powerpc64/bits/sem.h > @@ -0,0 +1,7 @@ > +struct semid_ds { > + struct ipc_perm sem_perm; > + time_t sem_otime; > + time_t sem_ctime; > + unsigned long sem_nsems; > + unsigned long __unused[2]; > +}; sem_nsems is required to have type unsigned short; this is a POSIX requirement that Linux botched. The canonical solution is to put endian-dependent padding around it; see other archs for examples. > diff --git a/arch/powerpc64/bits/signal.h b/arch/powerpc64/bits/signal.h > [...] > +typedef struct sigcontext > +{ > + unsigned long _unused[4]; > + int signal; > + int _pad0; > + unsigned long handler; > + unsigned long oldmask; > + void *regs; > + gregset_t gp_regs; > + fpregset_t fp_regs; > + vrregset_t *v_regs; > + long vmx_reserve[34+34+32+1]; > +} mcontext_t; > + > +#else > + > +typedef struct { > + long __regs[4+4+48+33+1+34+34+32+1]; > +} mcontext_t; > + > +#endif Did you check that the layout of the namespace-safe and full mcontex_t match? > +#define SA_NOCLDSTOP 1U > +#define SA_NOCLDWAIT 2U > +#define SA_SIGINFO 4U > +#define SA_ONSTACK 0x08000000U > +#define SA_RESTART 0x10000000U > +#define SA_NODEFER 0x40000000U > +#define SA_RESETHAND 0x80000000U > +#define SA_RESTORER 0x04000000U Is there a reason for making these unsigned? It's different from other archs at least, I think. > diff --git a/arch/powerpc64/crt_arch.h b/arch/powerpc64/crt_arch.h > new file mode 100644 > index 0000000..0605511 > --- /dev/null > +++ b/arch/powerpc64/crt_arch.h > @@ -0,0 +1,21 @@ > +__asm__( > +".text \n" > +".global " START " \n" > +".type " START ", %function \n" > +START ": \n" > +" addis 2, 12, .TOC.-" START "@ha \n" > +" addi 2, 2, .TOC.-" START "@l \n" What does this do? It looks like canonical function prologue of some sort, but _start is not a C function and unless r12 is part of the ELFv2 entry point ABI that I'm not aware of, I don't think it's meaningful. AFAICT r2 is not subsequently used. > diff --git a/arch/powerpc64/pthread_arch.h b/arch/powerpc64/pthread_arch.h > new file mode 100644 > index 0000000..2f976fe > --- /dev/null > +++ b/arch/powerpc64/pthread_arch.h > @@ -0,0 +1,17 @@ > +static inline struct pthread *__pthread_self() > +{ > + register char *tp __asm__("r13"); > + __asm__ __volatile__ ("" : "=r" (tp) ); > + return (pthread_t)(tp - 0x7000 - sizeof(struct pthread)); > +} It's possible this asm should have a "memory" clobber to avoid reordering across the initial thread pointer setup, but it's consistent with ppc32 right now, so we should probably review this as a separate issue to consider for both ports rather than something blocking the new port. > diff --git a/arch/powerpc64/reloc.h b/arch/powerpc64/reloc.h > new file mode 100644 > index 0000000..8e60b31 > --- /dev/null > +++ b/arch/powerpc64/reloc.h > @@ -0,0 +1,32 @@ > +#include > + > +#if __BYTE_ORDER == __LITTLE_ENDIAN > +#define ENDIAN_SUFFIX "le" > +#else > +#define ENDIAN_SUFFIX "" > +#endif > + > +#define LDSO_ARCH "powerpc64" ENDIAN_SUFFIX Is it intentional that the "default" subarch variant be suffixed with "le" and a non-default/unused one be the bare "powerpc64"? I don't object to that but it's contrary to usual conventions that the bare arch be the canonical ABI, and it might be contrary to what GCC is doing now (haven't checked) for the dynamic linker name. > diff --git a/arch/powerpc64/syscall_arch.h b/arch/powerpc64/syscall_arch.h > new file mode 100644 > index 0000000..aee11eb > --- /dev/null > +++ b/arch/powerpc64/syscall_arch.h > @@ -0,0 +1,5 @@ > +#define __SYSCALL_LL_E(x) (x) > +#define __SYSCALL_LL_O(x) (x) > + > +#undef SYSCALL_NO_INLINE > +#define SYSCALL_NO_INLINE Is this just unfinished or is there a reason inline syscalls don't work well? > diff --git a/configure b/configure > [...] > +if test "$ARCH" = "powerpc64" ; then > +if ! trycppif "_CALL_ELF == 2" "$t" ; then > +fail "$0: error: unsupported powerpc64 ABI" > +fi I usually use &&/|| for this kind of thing: trycppif "_CALL_ELF == 2" "$t" || fail ... > diff --git a/src/setjmp/powerpc64/longjmp.S b/src/setjmp/powerpc64/longjmp.S > new file mode 100644 > index 0000000..e82e2a7 > --- /dev/null > +++ b/src/setjmp/powerpc64/longjmp.S > @@ -0,0 +1,93 @@ > + .global _longjmp > + .global longjmp > + .type _longjmp,@function > + .type longjmp,@function > +_longjmp: > +longjmp: > + # 0) move old return address into the link register > + ld 0, 0*8(3) > + mtlr 0 > + # 1) restore cr > + ld 0, 1*8(3) > + mtcr 0 > + # 2) restore r1-r2 (SP and TOC) > + ld 1, 2*8(3) > + ld 2, 3*8(3) > + # 3) restore r14-r31 > + ld 14, 4*8(3) > + ld 15, 5*8(3) > + ld 16, 6*8(3) > + ld 17, 7*8(3) > + ld 18, 8*8(3) > + ld 19, 9*8(3) > + ld 20, 10*8(3) > + ld 21, 11*8(3) > + ld 22, 12*8(3) > + ld 23, 13*8(3) > + ld 24, 14*8(3) > + ld 25, 15*8(3) > + ld 26, 16*8(3) > + ld 27, 17*8(3) > + ld 28, 18*8(3) > + ld 29, 19*8(3) > + ld 30, 20*8(3) > + ld 31, 21*8(3) > + # 4) restore floating point registers f14-f31 > + lfd 14, 22*8(3) > + lfd 15, 23*8(3) > + lfd 16, 24*8(3) > + lfd 17, 25*8(3) > + lfd 18, 26*8(3) > + lfd 19, 27*8(3) > + lfd 20, 28*8(3) > + lfd 21, 29*8(3) > + lfd 22, 30*8(3) > + lfd 23, 31*8(3) > + lfd 24, 32*8(3) > + lfd 25, 33*8(3) > + lfd 26, 34*8(3) > + lfd 27, 35*8(3) > + lfd 28, 36*8(3) > + lfd 29, 37*8(3) > + lfd 30, 38*8(3) > + lfd 31, 39*8(3) > + > + # 5) restore vector registers v20-v31 > + addi 3, 3, 40*8 > + lvx 2, 0, 3 > + > +#if __BIG_ENDIAN__ > + lvsl 0, 0, 3 > +#define load_vr(cur,tmp1,tmp2) \ > + addi 3, 3, 16; \ > + lvx tmp2, 0, 3; \ > + vperm cur, tmp1, tmp2, 0 > +#else > + lvsr 0, 0, 3 > +#define load_vr(cur,tmp1,tmp2) \ > + addi 3, 3, 16; \ > + lvx tmp2, 0, 3; \ > + vperm cur, tmp2, tmp1, 0 > +#endif > + > + load_vr(20, 2, 3) > [...] This is kind of the reason why I was hesitant to add .S support for so long. :-) I don't want to reject it outright, but the idea of adding .S support was just to allow conditional compilation, not to do condensed assembly sources that require macro expansion. I can see where the code might be unwieldy without this though. Anyone else have opinions? > diff --git a/src/signal/powerpc64/sigsetjmp.s b/src/signal/powerpc64/sigsetjmp.s > new file mode 100644 > index 0000000..ce59b60 > --- /dev/null > +++ b/src/signal/powerpc64/sigsetjmp.s > @@ -0,0 +1,30 @@ > + .global sigsetjmp > + .global __sigsetjmp > + .type sigsetjmp,%function > + .type __sigsetjmp,%function > + .hidden ___setjmp > +sigsetjmp: > +__sigsetjmp: > + addis 2, 12, .TOC.-__sigsetjmp@ha > + addi 2, 2, .TOC.-__sigsetjmp@l > + .localentry sigsetjmp,.-sigsetjmp > + .localentry __sigsetjmp,.-__sigsetjmp Again I don't see what the purpose of these insns is; if the resulting value is needed, are you aware of how that interacts with ___setjmp returning twice? It looks to me like the assumption is that r12 contains the address of the callee at the time of a function call, but I don't see how that's satisfied in the calls I've seen. Other than that, things looked good! Obviously I haven't tested it yet, and I also haven't read some of the actual code in detail yet, so I don't know if there are other bugs; my review so far mostly covers design/style/conformance/policy matters. Rich