[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 29 Feb 2016 19:11:21 -0500
From: "Rich Felker (dalias@...c.org)" <dalias@...c.org>
To: Jaydeep Patil <Jaydeep.Patil@...tec.com>
Cc: Mahesh Bodapati <Mahesh.Bodapati@...tec.com>,
"musl@...ts.openwall.com" <musl@...ts.openwall.com>,
"nsz@...t70.net" <nsz@...t70.net>
Subject: Re: MUSL MIPS64 N64 port
On Fri, Feb 26, 2016 at 07:13:44AM +0000, Jaydeep Patil wrote:
> Hi Rich,
>
> Please refer to https://github.com/JaydeepIMG/musl-1 for MIPS64 N64 port.
> I have also attached the patch with this mail.
>
> Could you please find some time to review this?
Sure. FYI OpenWRT is already testing this and it seems to be working
for them.
First some cosmetics -- applying the patch with git apply reported
whitespace errors (lines with trailing whitespace) that should be
fixed. There are also some lines with incorrect indention (spaces).
Running the following commands on the patch file will find some of
them:
grep '. *$'
grep '^+ '
Also, comments should be left out in public headers (bits) and there
may be some space/tabs issues (tabs should be used for intention,
spaces for alignment) in comments in the asm (I didn't check yet).
> diff --git a/arch/mips64/bits/setjmp.h b/arch/mips64/bits/setjmp.h
> new file mode 100644
> index 0000000..fdf5df8
> --- /dev/null
> +++ b/arch/mips64/bits/setjmp.h
> @@ -0,0 +1 @@
> +typedef unsigned long long __jmp_buf[25];
Where does the size 25 come from? I don't have any reason to think
it's wrong but I just want to check since it's an ABI issue and one
we've messed up on early ports in the past.
> +typedef struct
> +{
> + gregset_t gregs;
> + fpregset_t fpregs;
> + greg_t mdhi;
> + greg_t hi1;
> + greg_t hi2;
> + greg_t hi3;
> + greg_t mdlo;
> + greg_t lo1;
> + greg_t lo2;
> + greg_t lo3;
> + greg_t pc;
> + unsigned int fpc_csr;
> + unsigned int used_math;
> + unsigned int dsp;
> + unsigned int reserved;
> +} mcontext_t;
Here's a place where you have spaces instead of tabs.
> diff --git a/arch/mips64/bits/socket.h b/arch/mips64/bits/socket.h
> new file mode 100644
> index 0000000..e83bcbb
> --- /dev/null
> +++ b/arch/mips64/bits/socket.h
> @@ -0,0 +1,72 @@
> +#include <endian.h>
> +
> +struct msghdr
> +{
> + void *msg_name;
> + socklen_t msg_namelen;
> + struct iovec *msg_iov;
> +#if __BYTE_ORDER == __BIG_ENDIAN
> + int __pad1, msg_iovlen;
> +#else
> + int msg_iovlen, __pad1;
> +#endif
> + void *msg_control;
> +#if __BYTE_ORDER == __BIG_ENDIAN
> + int __pad2;
> + socklen_t msg_controllen;
> +#else
> + socklen_t msg_controllen;
> + int __pad2;
> +#endif
> + int msg_flags;
> +};
Here too.
> diff --git a/arch/mips64/bits/stat.h b/arch/mips64/bits/stat.h
> new file mode 100644
> index 0000000..1f1bbaa
> --- /dev/null
> +++ b/arch/mips64/bits/stat.h
> @@ -0,0 +1,25 @@
> +#include <string.h>
> +
> +#include <bits/alltypes.h>
> +
> +struct stat {
> + dev_t st_dev;
> + int st_pad1[3];
> + ino_t st_ino; /* File serial number. */
> + mode_t st_mode; /* File mode. */
> + nlink_t st_nlink; /* Link count. */
> + uid_t st_uid; /* User ID of the file's owner. */
> + gid_t st_gid; /* Group ID of the file's group.*/
> + dev_t st_rdev; /* Device number, if device. */
> + unsigned int st_pad2[2];
> + off_t st_size; /* Size of file, in bytes. */
> + int st_pad3;
> + struct timespec st_atim; /* Time of last access. */
> + struct timespec st_mtim; /* Time of last modification. */
> + struct timespec st_ctim; /* Time of last status change. */
> + blksize_t st_blksize; /* Optimal block size for I/O. */
> + unsigned int st_pad4;
> + blkcnt_t st_blocks; /* Number of 512-byte blocks allocated. */
> + int st_pad5[14];
> +};
And here, plus comments that should be removed.
Also the padding should not have names in public st_* namespace but
rather just __padN or similar.
> diff --git a/arch/mips64/bits/stdarg.h b/arch/mips64/bits/stdarg.h
> new file mode 100644
> index 0000000..fde3781
> --- /dev/null
> +++ b/arch/mips64/bits/stdarg.h
> @@ -0,0 +1,4 @@
> +#define va_start(v,l) __builtin_va_start(v,l)
> +#define va_end(v) __builtin_va_end(v)
> +#define va_arg(v,l) __builtin_va_arg(v,l)
> +#define va_copy(d,s) __builtin_va_copy(d,s)
This file can be omitted completely.
> diff --git a/arch/mips64/bits/stdint.h b/arch/mips64/bits/stdint.h
> new file mode 100644
> index 0000000..0159d88
> --- /dev/null
> +++ b/arch/mips64/bits/stdint.h
> @@ -0,0 +1,20 @@
> +typedef int32_t int_fast16_t;
> +typedef int32_t int_fast32_t;
> +typedef uint32_t uint_fast16_t;
> +typedef uint32_t uint_fast32_t;
> +
> +#define INT_FAST16_MIN INT32_MIN
> +#define INT_FAST32_MIN INT32_MIN
> +
> +#define INT_FAST16_MAX INT32_MAX
> +#define INT_FAST32_MAX INT32_MAX
> +
> +#define UINT_FAST16_MAX UINT32_MAX
> +#define UINT_FAST32_MAX UINT32_MAX
> +
> +#define INTPTR_MIN INT64_MIN //size of pointer is 8 bytes .address is 64 bit
Comments like this can be omitted; they don't say anything that the
code doesn't already say and we try to avoid comments in installed
headers too.
> diff --git a/arch/mips64/bits/syscall.h b/arch/mips64/bits/syscall.h
> new file mode 100644
> index 0000000..6407d92
> --- /dev/null
> +++ b/arch/mips64/bits/syscall.h
> @@ -0,0 +1,644 @@
> +/*
> + * Linux 64-bit syscalls are in the range from 5000 to 5999.
> + */
> +#define __NR_Linux 5000
> +#define __NR_read (__NR_Linux + 0)
> +#define __NR_write (__NR_Linux + 1)
> [...]
I would prefer doing like the existing 32-bit mips port and just
expanding the 5000, 5001, etc. directly. Use of the __NR_linux macro
just makes the source tree larger and makes it take longer to parse
and preprocess; it doesn't make it any more readable.
> diff --git a/arch/mips64/crt_arch.h b/arch/mips64/crt_arch.h
> new file mode 100644
> index 0000000..feb668a
> --- /dev/null
> +++ b/arch/mips64/crt_arch.h
> @@ -0,0 +1,33 @@
> +__asm__(
> +".set push\n"
> +".set noreorder\n"
> +".text \n"
> +".global _" START "\n"
> +".global " START "\n"
> +".global " START "_data\n"
> +".type _" START ", @function\n"
> +".type " START ", @function\n"
> +".type " START "_data, @function\n"
> +"_" START ":\n"
> +"" START ":\n"
> +".align 8 \n"
> +" bal 1f \n"
> +" move $fp, $0 \n"
> +"" START "_data: \n"
> +" .gpdword " START "_data \n"
> +" .gpdword " START "_c \n"
> +".weak _DYNAMIC \n"
> +".hidden _DYNAMIC \n"
> +" .gpdword _DYNAMIC \n"
> +"1: ld $gp, 0($ra) \n"
> +" dsubu $gp, $ra, $gp \n"
> +" move $4, $sp \n"
> +" ld $5, 16($ra) \n"
> +" daddu $5, $5, $gp \n"
> +" ld $25, 8($ra) \n"
> +" daddu $25, $25, $gp \n"
> +" and $sp, $sp, -16 \n"
> +" jalr $25 \n"
> +" nop \n"
> +".set pop \n"
> +);
Inconsistent mix of tabs/spaces; should be all tabs for indenting.
> diff --git a/arch/mips64/ksigaction.h b/arch/mips64/ksigaction.h
> new file mode 100644
> index 0000000..0197686
> --- /dev/null
> +++ b/arch/mips64/ksigaction.h
> @@ -0,0 +1,11 @@
> +struct k_sigaction {
> + unsigned flags;
> + void (*handler)(int);
> + unsigned long mask[2]; /*mask [128/(sizeof(long)*8)]
> + /* The following field is past the end of the structure the
> + * kernel will read or write, and exists only to avoid having
> + * mips-specific preprocessor conditionals in sigaction.c. */
> + void (*restorer)();
The commented out oversized mask should be removed. I suspect
compilers will give a nested-comment warning for it as-is.
> diff --git a/arch/mips64/reloc.h b/arch/mips64/reloc.h
This should also have mips-specific R_* defs; see below.
> diff --git a/arch/mips64/syscall_arch.h b/arch/mips64/syscall_arch.h
> new file mode 100644
> index 0000000..19d175d
> --- /dev/null
> +++ b/arch/mips64/syscall_arch.h
> @@ -0,0 +1,213 @@
> +#define __SYSCALL_LL_E(x) (x)
> +#define __SYSCALL_LL_O(x) (x)
> +
> +__attribute__((visibility("hidden")))
> +long (__syscall)(long, ...);
> +
> +#define SYSCALL_RLIM_INFINITY (-1UL/2)
> +
> +#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;
> +};
Also has spaces instead of tabs.
> +
> +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;
> +}
I don't think the memsets are necessary, and they're going to make the
calling function big/messy because they add extra external calls.
> +static inline long __syscall3(long n, long a, long b, long c)
> +{
> + register long r4 __asm__("$4") = a;
> + register long r5 __asm__("$5") = b;
> + register long r6 __asm__("$6") = c;
> + 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), "r"(r5), "r"(r6)
> + : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> + "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> + return r7 ? -r2 : r2;
> +}
> +
> +static inline long __syscall4(long n, long a, long b, long c, long d)
> +{
> + register long r4 __asm__("$4") = a;
> + register long r5 __asm__("$5") = b;
> + register long r6 __asm__("$6") = c;
> + register long r7 __asm__("$7") = d;
> + register long r2 __asm__("$2");
> + __asm__ __volatile__ (
> + "daddu $2,$0,%2 ; syscall"
> + : "=&r"(r2), "=r"(r7) : "ir"(n), "0"(r2), "1"(r7),
> + "r"(r4), "r"(r5), "r"(r6)
> + : "$1", "$3", "$8", "$9", "$10", "$11", "$12", "$13",
> + "$14", "$15", "$24", "$25", "hi", "lo", "memory");
> + return r7 ? -r2 : r2;
> +}
All __syscallN for N>=2 should do the stat fix (as on 32-bit mips);
otherwise, adding unused args to the syscalls would break. This
especially affects src/misc/syscall.c which provides the public
syscall() function. The compiler will optimize out the code anyway
when n is a constant.
> diff --git a/src/internal/dynlink.h b/src/internal/dynlink.h
> index 48890b2..73eeaba 100644
> --- a/src/internal/dynlink.h
> +++ b/src/internal/dynlink.h
> @@ -11,12 +11,25 @@ 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
> +#endif
This part looks fine.
> +#ifdef __mips64
> +#define _GNU_SOURCE
> +#include <endian.h>
> +#undef R_TYPE
> +#undef R_SYM
> +#undef R_INFO
> +#define R_TYPE(x) (be64toh(x)&0x7fffffff)
> +#define R_SYM(x) (be32toh(be64toh(x)>>32))
> +#define R_INFO(s,t) (htobe64((uint64_t)htobe32(s)<<32 | (uint64_t)t))
> #endif
This should be moved into arch/mips64/reloc.h rather than using an
arch-specific #ifdef in a shared file.
Also _GNU_SOURCE cannot be defined "late"; feature test macros have to
appear before any system header is included. You can just assume the
file including it has defined whatever's needed to make the endian.h
functions visible, I think. Let me know if that's not working.
That's all I saw for now. I'm also running a build and test of my own
right now so I'll let you know if I see anything else that doesn't
look right.
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.
