[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160327233709.GE21636@brightrain.aerifal.cx>
Date: Sun, 27 Mar 2016 19:37:09 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 2/2] add powerpc64 port
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 <endian.h>
> +
> +#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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
