Re: MUSL MIPS64 N64 port

["Rich Felker (dalias@libc.org)" <dalias@libc.org>]

On Tue, Mar 01, 2016 at 07:52:33AM +0000, Jaydeep Patil wrote:
> Also got:
> 
> src/thread/pthread_create.c:212:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>    stack = (void *)(attr._a_stackaddr & -16);
> 
> This is because your pthread types are wrong; you're using the 32-bit arch ones:

This does not seem to be fixed; see below in your new patch:

> diff --git a/arch/mips64/bits/alltypes.h.in b/arch/mips64/bits/alltypes.h.in
> new file mode 100644
> index 0000000..64385c9
> --- /dev/null
> +++ b/arch/mips64/bits/alltypes.h.in
> [...]
> +TYPEDEF struct { union { int __i[9]; volatile int __vi[9]; unsigned long __s[9]; } __u; } pthread_attr_t;
> +TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
> +TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
> +TYPEDEF struct { union { int __i[12]; volatile int __vi[12]; void *__p[12]; } __u; } pthread_cond_t;
> +TYPEDEF struct { union { int __i[12]; volatile int __vi[12]; void *__p[12]; } __u; } cnd_t;
> +TYPEDEF struct { union { int __i[8]; volatile int __vi[8]; void *__p[8]; } __u; } pthread_rwlock_t;
> +TYPEDEF struct { union { int __i[5]; volatile int __vi[5]; void *__p[5]; } __u; } pthread_barrier_t;

These are still the 32-bit definitions not the 64-bit ones.

> diff --git a/arch/mips64/bits/stat.h b/arch/mips64/bits/stat.h
> new file mode 100644
> index 0000000..dfc80b3
> --- /dev/null
> +++ b/arch/mips64/bits/stat.h
> @@ -0,0 +1,23 @@
> +#include <string.h>
> +#include <bits/alltypes.h>
> +
> +struct stat {
> +	dev_t st_dev;
> +	int pad1[3];
> +	ino_t st_ino;
> +	mode_t st_mode;
> +	nlink_t st_nlink;
> +	uid_t st_uid;
> +	gid_t st_gid;
> +	dev_t st_rdev;
> +	unsigned int pad2[2];
> +	off_t st_size;
> +	int pad3;
> +	struct timespec st_atim;
> +	struct timespec st_mtim;
> +	struct timespec st_ctim;
> +	blksize_t st_blksize;
> +	unsigned int pad4;
> +	blkcnt_t st_blocks;
> +	int pad5[14];
> +};

pad1, pad2 etc. are in the application-reserved namespace. These need
to be __pad1, __pad2, etc. or similar. -- something reserved.

> +static void __stat_fix(struct kernel_stat *kst, struct stat *st)
> +{
> +	st->st_dev = kst->st_dev;
> +	st->pad1[0] = 0;
> +	st->pad1[1] = 0;
> +	st->pad1[2] = 0;
> +	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;
> +	st->pad2[0] = 0;
> +	st->pad2[1] = 0;
> +	st->st_size = kst->st_size;
> +	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;
> +	return;
> +}

When I said the memset can be removed I didn't mean to replace it with
assignments. Although they're not as bad, I don't see any reason
they're needed at all. The padding does not need to contain meaningful
content. Just leaving it uninitialized is fine.

Also, the return statement is not needed.

> +static inline long __syscall2(long n, long a, long b)
> +{
> +	struct kernel_stat kst = {0,};

I didn't catch this before, but is there a reason you're filling kst
with zeros before passing it to the kernel? I don't think it's useful,
and it generates extra code the compiler cannot optimize out (because
it cannot know the kernel won't read the memory). Same applies for all
the oher __syscallN's.

The rest of your changes all look okay.

Since it looks like it's about ready to commit, I have a couple
general questions about merging. How should the port be credited? To
imgtec.com? Should someone on your team be the commit author in the
commit, or should I just commit it in my name with your team credited
in the commit message and COPYRIGHT file?

Thanks for your work on this and your patience with review. :)

Rich