[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20160223033233.GM9349@brightrain.aerifal.cx>
Date: Mon, 22 Feb 2016 22:32:33 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: mips n64 porting review
On Mon, Feb 22, 2016 at 10:06:02AM +0000, Mahesh Bodapati wrote:
> Hi Rich,
> I have attached the patch which has all the MIPS n64 porting work. I
> have created mipsn64port_review remote branch on GitHub and please
> have a look at
> https://github.com/MaheshBodapati/musl/commits/mipsn64port_review
> which has the broken down patches and the base revision on which we
> have prepared patch is d150764
>
> We have tested on both little endian and big endian, here are the FAILS
>
> Musl mips n64 libc testing on EL :
>
> Fails: 17
> FAIL ./src/api/main.exe [status 1]
> FAIL ./src/functional/wcstol-static.exe [status 1]
> FAIL ./src/functional/wcstol.exe [status 1]
> FAIL ./src/math/acosh.exe [status 1]
> FAIL ./src/math/asinh.exe [status 1]
> FAIL ./src/math/j0.exe [status 1]
> FAIL ./src/math/jn.exe [status 1]
> FAIL ./src/math/jnf.exe [status 1]
> FAIL ./src/math/lgamma.exe [status 1]
> FAIL ./src/math/lgamma_r.exe [status 1]
> FAIL ./src/math/lgammaf.exe [status 1]
> FAIL ./src/math/lgammaf_r.exe [status 1]
> FAIL ./src/math/sinh.exe [status 1]
> FAIL ./src/math/tgamma.exe [status 1]
> FAIL ./src/math/y0.exe [status 1]
> FAIL ./src/math/y0f.exe [status 1]
> FAIL ./src/math/ynf.exe [status 1]
>
> Musl mips n64 libc testing on EB :
> Fails:17
> FAIL ./src/api/main.exe [status 1]
> FAIL ./src/functional/wcstol-static.exe [status 1]
> FAIL ./src/functional/wcstol.exe [status 1]
> FAIL ./src/math/acosh.exe [status 1]
> FAIL ./src/math/asinh.exe [status 1]
> FAIL ./src/math/j0.exe [status 1]
> FAIL ./src/math/jn.exe [status 1]
> FAIL ./src/math/jnf.exe [status 1]
> FAIL ./src/math/lgamma.exe [status 1]
> FAIL ./src/math/lgamma_r.exe [status 1]
> FAIL ./src/math/lgammaf.exe [status 1]
> FAIL ./src/math/lgammaf_r.exe [status 1]
> FAIL ./src/math/sinh.exe [status 1]
> FAIL ./src/math/tgamma.exe [status 1]
> FAIL ./src/math/y0.exe [status 1]
> FAIL ./src/math/y0f.exe [status 1]
> FAIL ./src/math/ynf.exe [status 1]
These look fairly expected.
> diff --git a/arch/mips/syscall_arch.h b/arch/mips/syscall_arch.h
^^^^^^^^^
This is changing the existing mips port (not mips64)...
> index 39c0ea3..8097180 100644
> --- a/arch/mips/syscall_arch.h
> +++ b/arch/mips/syscall_arch.h
> @@ -54,15 +54,16 @@ static inline long __syscall2(long n, long a, long b)
> register long r5 __asm__("$5") = b;
> register long r7 __asm__("$7");
> register long r2 __asm__("$2");
> + long ret;
> __asm__ __volatile__ (
> "addu $2,$0,%2 ; syscall"
> : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> "r"(r4), "r"(r5)
> : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> - if (r7) return -r2;
> - long ret = r2;
> - if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
> + ret = r7 ? -r2 : r2;
> + if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64)
> + __stat_fix (b);
> return ret;
> }
>
> @@ -79,10 +80,7 @@ static inline long __syscall3(long n, long a, long b, long c)
> "r"(r4), "r"(r5), "r"(r6)
> : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> - if (r7) return -r2;
> - long ret = r2;
> - if (n == SYS_stat64 || n == SYS_fstat64 || n == SYS_lstat64) __stat_fix(b);
> - return ret;
> + return r7 ? -r2 : r2;
> }
...and significantly breaking it.
> diff --git a/arch/mips64/atomic_arch.h b/arch/mips64/atomic_arch.h
> new file mode 100644
> index 0000000..d90442d
> --- /dev/null
> +++ b/arch/mips64/atomic_arch.h
> @@ -0,0 +1,221 @@
> +#define a_cas a_cas
> +static inline int a_cas(volatile int *p, int t, int s)
> +{
> + int dummy;
> + __asm__ __volatile__(
> + ".set push\n"
> + ".set noreorder\n"
> + " sync\n"
> + "1: ll %0, %2\n"
> + " bne %0, %3, 1f\n"
> + " addu %1, %4, $0\n"
> + " sc %1, %2\n"
> + " beq %1, $0, 1b\n"
> + " nop\n"
> + " sync\n"
> + "1: \n"
> + ".set pop\n"
> + : "=&r"(t), "=&r"(dummy), "+m"(*p) : "r"(t), "r"(s) : "memory" );
> + return t;
> +}
> [...]
As noted by Bobby Bingham, you can just define the new ll/sc fragments
rather than big asm blocks like were needed in old musl versions. I'll
try to get the right infrastructure for the 64-bit pointer variant
upstream right away so you can use it without needing more duplication
in the mips64 port; right now it's just in the aarch64 dir.
> diff --git a/arch/mips64/bits/float.h b/arch/mips64/bits/float.h
> new file mode 100644
> index 0000000..719c790
> --- /dev/null
> +++ b/arch/mips64/bits/float.h
> @@ -0,0 +1,16 @@
> +#define FLT_EVAL_METHOD 0
> +
> +#define LDBL_TRUE_MIN 6.47517511943802511092443895822764655e-4966L
> +#define LDBL_MIN 3.36210314311209350626267781732175260e-4932L
> +#define LDBL_MAX 1.18973149535723176508575932662800702e+4932L
> +#define LDBL_EPSILON 1.92592994438723585305597794258492732e-34L
> +
> +#define LDBL_MANT_DIG 113
> +#define LDBL_MIN_EXP (-16381)
> +#define LDBL_MAX_EXP 16384
> +
> +#define LDBL_DIG 33
> +#define LDBL_MIN_10_EXP (-4931)
> +#define LDBL_MAX_10_EXP 4932
> +
> +#define DECIMAL_DIG 36
Is your mips64 port using IEEE quad? These values look like they're
matched to IEEE quad, but I couldn't find clear answers on whether
mips64 is using IEEE quad or double-double. The latter cannot be
supported, so if that's what it's using, we'd need to either find a
way to configure the compiler for quad or for 64-bit long double.
> diff --git a/arch/mips64/bits/sem.h b/arch/mips64/bits/sem.h
> new file mode 100644
> index 0000000..1c05a22
> --- /dev/null
> +++ b/arch/mips64/bits/sem.h
> @@ -0,0 +1,8 @@
> +struct semid_ds {
> + struct ipc_perm sem_perm;
> + time_t sem_otime;
> + time_t sem_ctime;
> + unsigned long sem_nsems;
> + unsigned long __unused3;
> + unsigned long __unused4;
> +};
sem_nsems has to have type unsigned short (this is a POSIX
requirement), with appropriate padding to match the kernel's wrong
definition. See how it's done on other archs including 32-bit mips.
The padding will need to be endian-dependent.
> diff --git a/arch/mips64/pthread_arch.h b/arch/mips64/pthread_arch.h
> new file mode 100644
> index 0000000..b42edbe
> --- /dev/null
> +++ b/arch/mips64/pthread_arch.h
> @@ -0,0 +1,18 @@
> +static inline struct pthread *__pthread_self()
> +{
> +#ifdef __clang__
> + char *tp;
> + __asm__ __volatile__ (".word 0x7c03e83b ; move %0, $3" : "=r" (tp) : : "$3" );
> +#else
> + register char *tp __asm__("$3");
> + __asm__ __volatile__ (".word 0x7c03e83b" : "=r" (tp) );
> +#endif
> + return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
> +}
Not critical, but it would be nice to know if clang still needs this
pessimization or if there's a way to make it use the right register
directly.
> diff --git a/arch/mips64/syscall_arch.h b/arch/mips64/syscall_arch.h
> new file mode 100644
> index 0000000..380b3fb
> --- /dev/null
> +++ b/arch/mips64/syscall_arch.h
> @@ -0,0 +1,205 @@
> +#define __SYSCALL_LL_E(x) \
> +((union { long long ll; long l[2]; }){ .ll = x }).l[0], \
> +((union { long long ll; long l[2]; }){ .ll = x }).l[1]
> +#define __SYSCALL_LL_O(x) 0, __SYSCALL_LL_E((x))
As Bobby Bingham noted these should both just be defined as (x) for
64-bit archs.
> +#define SYSCALL_RLIM_INFINITY (-1UL/2)
Is this right for mips64?
> +#include <sys/stat.h>
> +struct kernel_stat {
> + unsigned int st_dev;
> + unsigned int __pad1[3];
> + unsigned long long st_ino;
> + unsigned int st_mode;
> + unsigned int st_nlink;
> + int st_uid;
> + int st_gid;
> + unsigned int st_rdev;
> + unsigned int __pad2[3];
> + long long st_size;
> + unsigned int st_atime_sec;
> + unsigned int st_atime_nsec;
> + unsigned int st_mtime_sec;
> + unsigned int st_mtime_nsec;
> + unsigned int st_ctime_sec;
> + unsigned int st_ctime_nsec;
> + unsigned int st_blksize;
> + unsigned int __pad3;
> + unsigned long long st_blocks;
> +};
> +
> +static void __stat_fix(struct kernel_stat *kst, struct stat *st)
> +{
> + extern void *memset(void *s, int c, size_t n);
> +
> + st->st_dev = kst->st_dev;
> + memset (&st->st_pad1, 0, sizeof (st->st_pad1));
> + st->st_ino = kst->st_ino;
> + st->st_mode = kst->st_mode;
> + st->st_nlink = kst->st_nlink;
> + st->st_uid = kst->st_uid;
> + st->st_gid = kst->st_gid;
> + st->st_rdev = kst->st_rdev;
> + memset (&st->st_pad2, 0, sizeof (st->st_pad2));
> + st->st_size = kst->st_size;
> + st->st_pad3 = 0;
> + st->st_atim.tv_sec = kst->st_atime_sec;
> + st->st_atim.tv_nsec = kst->st_atime_nsec;
> + st->st_mtim.tv_sec = kst->st_mtime_sec;
> + st->st_mtim.tv_nsec = kst->st_mtime_nsec;
> + st->st_ctim.tv_sec = kst->st_ctime_sec;
> + st->st_ctim.tv_nsec = kst->st_ctime_nsec;
> + st->st_blksize = kst->st_blksize;
> + st->st_blocks = kst->st_blocks;
> + memset (&st->st_pad5, 0, sizeof (st->st_pad5));
> + return;
> +}
You've made this a lot more complicated than it needs to be. Just
copying the __stat_fix code from 32-bit mips should work fine. The
only fixup that needs to be done is for st_dev and st_rdev and it can
be done in-place.
> +
> +#ifndef __clang__
> +
> +static inline long __syscall0(long n)
> +{
> + register long r7 __asm__("$7");
> + register long r2 __asm__("$2");
> + __asm__ __volatile__ (
> + "daddu $2,$0,%2 ; syscall"
> + : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7)
> + : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> + "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> + return r7 ? -r2 : r2;
> +}
> +
> +static inline long __syscall1(long n, long a)
> +{
> + register long r4 __asm__("$4") = a;
> + register long r7 __asm__("$7");
> + register long r2 __asm__("$2");
> + __asm__ __volatile__ (
> + "daddu $2,$0,%2 ; syscall"
> + : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> + "r"(r4)
> + : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> + "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> + return r7 ? -r2 : r2;
> +}
> +
> +static inline long __syscall2(long n, long a, long b)
> +{
> + struct kernel_stat kst = {0,};
> + long ret;
> + register long r4 __asm__("$4");
> + register long r5 __asm__("$5");
> + register long r7 __asm__("$7");
> + register long r2 __asm__("$2");
> +
> + r5 = b;
> + if (n == SYS_stat || n == SYS_fstat || n == SYS_lstat)
> + r5 = (long) &kst;
> +
> + r4 = a;
> + __asm__ __volatile__ (
> + "daddu $2,$0,%2 ; syscall"
> + : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> + "r"(r4), "r"(r5)
> + : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> + "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> +
> + ret = r7 ? -r2 : r2;
> + if (n == SYS_stat || n == SYS_fstat || n == SYS_lstat)
> + __stat_fix(&kst, (struct stat *)b);
> +
> + return ret;
> +}
See the logic on 32-bit mips. There should be an early return without
performing __stat_fix in the error case (if r7 is nonzero). Otherwise
you're performing fixups on uninitialized data.
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index 87f3b7f..2355d74 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -325,8 +325,8 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
> for (; rel_size; rel+=stride, rel_size-=stride*sizeof(size_t)) {
> if (skip_relative && IS_RELATIVE(rel[1], dso->syms)) continue;
> type = R_TYPE(rel[1]);
> - if (type == REL_NONE) continue;
> sym_index = R_SYM(rel[1]);
> + if (type == REL_NONE) continue;
This looks gratuitous and probably just leftover from old things you tried.
> @@ -1134,7 +1134,7 @@ static void do_mips_relocs(struct dso *p, size_t *got)
> Sym *sym = p->syms + j;
> rel[0] = (unsigned char *)got - base;
> for (i-=j; i; i--, sym++, rel[0]+=sizeof(size_t)) {
> - rel[1] = sym-p->syms << 8 | R_MIPS_JUMP_SLOT;
> + rel[1] = R_INFO(sym-p->syms, R_MIPS_JUMP_SLOT);
> do_relocs(p, rel, sizeof rel, 2);
> }
> }
Looks ok.
> @@ -1365,7 +1365,7 @@ void __dls2(unsigned char *base, size_t *sp)
> size_t symbolic_rel_cnt = 0;
> apply_addends_to = rel;
> for (; rel_size; rel+=2, rel_size-=2*sizeof(size_t))
> - if (!IS_RELATIVE(rel[1], ldso.syms)) symbolic_rel_cnt++;
> + if (!IS_RELATIVE(rel[1], ldso.syms)) symbolic_rel_cnt++;
This looks like a non-functional change, but wrong (misleading
indention) and probably unintentional.
> diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> index 48890b2..dcc1bde 100644
> --- a/src/internal/dynlink.h
> +++ b/src/internal/dynlink.h
> @@ -11,14 +11,49 @@ typedef Elf32_Phdr Phdr;
> typedef Elf32_Sym Sym;
> #define R_TYPE(x) ((x)&255)
> #define R_SYM(x) ((x)>>8)
> +#define R_INFO ELF32_R_INFO
> #else
> typedef Elf64_Ehdr Ehdr;
> typedef Elf64_Phdr Phdr;
> typedef Elf64_Sym Sym;
> #define R_TYPE(x) ((x)&0x7fffffff)
> #define R_SYM(x) ((x)>>32)
> +#define R_INFO ELF64_R_INFO
> +#define ELF64 1
> #endif
I don't think ELF64 is used anywhere so it can be removed.
> +#ifdef __mips64
> +#undef R_TYPE
> +#undef R_SYM
> +#undef R_INFO
> +#define R_TYPE(INFO) ({ \
> + uint64_t r_info = (INFO); \
> + uint32_t *type, r_type; \
> + type = (((uint32_t*) &r_info) + 1); \
> + r_type = (uint32_t) *(((unsigned char*) type) + 3) | \
> + (uint32_t) *(((unsigned char*) type) + 2) << 8 | \
> + (uint32_t) *(((unsigned char*) type) + 1) << 16; \
> + r_type; \
> +})
> +
> +#define R_SYM(INFO) ({ \
> + uint64_t r_info = (INFO); \
> + uint32_t symidx; \
> + symidx = *(((uint32_t*) &r_info) + 0); \
> + symidx; \
> +})
> +
> +#define R_INFO(SYM,TYPE1) ({ \
> + uint32_t *pinfo; \
> + uint64_t r_info = 0; \
> + pinfo = ((uint32_t*) &r_info) + 1; \
> + *((uint32_t*) &r_info + 0) = (SYM); \
> + *((unsigned char*) pinfo + 3) = (TYPE1); \
> + r_info; \
> +})
> +#endif
At the very least this is buggy (aliasing violations) and gratuitously
ugly, and might be wrong for little-endian. Have you tested it on
little?
This code above implies that the values are always stored in
big-endian order as (sym<<32 | type). If they're actually stored
native-endian (which I would expect) then the standard Elf64
definitions just work as-is and don't need replacement. If they're
always-big, tweaked versions of the standard ones that swap the byte
order of their inputs or outputs would be all that's needed.
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.
