[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160302044143.GM9349@brightrain.aerifal.cx>
Date: Tue, 1 Mar 2016 23:41:43 -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 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
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.
