[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 2 Mar 2016 05:50:41 +0000
From: Jaydeep Patil <Jaydeep.Patil@...tec.com>
To: "Rich Felker (dalias@...c.org)" <dalias@...c.org>
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
>-----Original Message-----
>From: Rich Felker [mailto:dalias@...ifal.cx] On Behalf Of Rich Felker
>(dalias@...c.org)
>Sent: 02 March 2016 AM 10:12
>To: Jaydeep Patil
>Cc: Mahesh Bodapati; musl@...ts.openwall.com; nsz@...t70.net
>Subject: Re: [musl] 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.
>
Ok.
>> 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.
>
Ok.
>> +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.
>
Ok.
>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.
>
Ok.
Please refer to https://github.com/JaydeepIMG/musl-1 for MIPS64 N64 port.
I have created mips64port_v3 branch (patch attached) to address the review comments.
>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?
>
You can credit it to Imagination Technologies.
Could you please commit it with "Mahesh Bodapati and Jaydeep Patil" in the message?
>Thanks for your work on this and your patience with review. :)
>
>Rich
Thanks for your comments :)
Regards,
Jaydeep
Download attachment "mipsn64port_v3.patch" of type "application/octet-stream" (72613 bytes)
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.
