From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9394 Path: news.gmane.org!not-for-mail From: Jaydeep Patil Newsgroups: gmane.linux.lib.musl.general Subject: RE: mips n64 porting review Date: Fri, 26 Feb 2016 03:58:47 +0000 Message-ID: References: <20160223033233.GM9349@brightrain.aerifal.cx> <20160225180150.GT9349@brightrain.aerifal.cx> <20160225182233.GU9349@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1456459154 12253 80.91.229.3 (26 Feb 2016 03:59:14 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 26 Feb 2016 03:59:14 +0000 (UTC) Cc: Mahesh Bodapati To: "dalias@libc.org" , "musl@lists.openwall.com" Original-X-From: musl-return-9407-gllmg-musl=m.gmane.org@lists.openwall.com Fri Feb 26 04:59:07 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1aZ9YZ-0002YZ-94 for gllmg-musl@m.gmane.org; Fri, 26 Feb 2016 04:59:07 +0100 Original-Received: (qmail 1915 invoked by uid 550); 26 Feb 2016 03:59:03 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 1891 invoked from network); 26 Feb 2016 03:59:03 -0000 Thread-Topic: [musl] mips n64 porting review Thread-Index: AQHRbgMhKb+EoLrPAE+MXWa3enS5GZ88pTOwgAAESwCAAG2Rf4AAoJ6w In-Reply-To: <20160225182233.GU9349@brightrain.aerifal.cx> Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.93.60] Xref: news.gmane.org gmane.linux.lib.musl.general:9394 Archived-At: Hi Rich, Thanks for the comments. I will submit the N64 patch with nlink_t changed. Regards, Jaydeep > -----Original Message----- > From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of dalias@libc.org > Sent: 25 February 2016 PM 11:53 > To: musl@lists.openwall.com > Cc: Jaydeep Patil; Mahesh Bodapati > Subject: Re: [musl] mips n64 porting review >=20 > Sorry for dropping the CC. More follow-up below: >=20 > On Thu, Feb 25, 2016 at 01:01:50PM -0500, dalias@libc.org wrote: > > On Thu, Feb 25, 2016 at 12:05:33PM +0000, Jaydeep Patil wrote: > > > >> +#include > > > >> +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 =3D kst->st_dev; > > > >> + memset (&st->st_pad1, 0, sizeof (st->st_pad1)); > > > >> + st->st_ino =3D kst->st_ino; > > > >> + st->st_mode =3D kst->st_mode; > > > >> + st->st_nlink =3D kst->st_nlink; > > > >> + st->st_uid =3D kst->st_uid; > > > >> + st->st_gid =3D kst->st_gid; > > > >> + st->st_rdev =3D kst->st_rdev; > > > >> + memset (&st->st_pad2, 0, sizeof (st->st_pad2)); > > > >> + st->st_size =3D kst->st_size; > > > >> + st->st_pad3 =3D 0; > > > >> + st->st_atim.tv_sec =3D kst->st_atime_sec; > > > >> + st->st_atim.tv_nsec =3D kst->st_atime_nsec; > > > >> + st->st_mtim.tv_sec =3D kst->st_mtime_sec; > > > >> + st->st_mtim.tv_nsec =3D kst->st_mtime_nsec; > > > >> + st->st_ctim.tv_sec =3D kst->st_ctime_sec; > > > >> + st->st_ctim.tv_nsec =3D kst->st_ctime_nsec; > > > >> + st->st_blksize =3D kst->st_blksize; > > > >> + st->st_blocks =3D 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. > > > > > > In case of MIPS64, the stat syscall returns a structure as described > > > by 'struct kernel_stat'. The ' struct stat' in MIPS64 has following > > > notable difference as compared to 'struct kernel_stat': > > > > > > The st_nlink is of 8 bytes > > > > I missed that in my previous reading. > > > > For all existing archs, nlink_t was the native register size (_Reg) in > > the kernel stat struct. It looks like it's just 32-bit (unsigned int) > > on mips64 though. The clean way to handle this is just moving nlink_t > > back to the per-arch alltypes.h.in files to allow it to vary (or > > actually, just putting a definition in the mips one will override the > > generic one). So in arch/mips64/bits/alltypes.h.in: > > > > TYPEDEF unsigned nlink_t; > > > > > and all st_*tim members are of 16 bytes > > > > Uhg. Does mips64 actually use a 32-bit time_t? If so I think with > > current musl we're stuck as defining time_t as 32-bit, because it's > > used as an interface with all sorts of things in the kernel that we > > don't wrap/control. But this is going to need a lot more review to > > understand if there's any better solution. :( > > > > Even if time_t is 32-bit though, tv_nsec is always long, so the > > kernel's stat structure is broken and needs further fixups like you're > > doing. I don't see any way around this unless there's hope of the > > kernel 64-bit time_t stuff being merged soon, in which case we could > > use that from the beginning on mips64. It feels really awful to add a > > new 64-bit arch with 32-bit time_t. > > > > In light of this I'm not sure it makes sense to try to match the > > kernel's weird nlink_t (see above). >=20 > I've done further reading of what glibc and the kernel do, and it's confu= sing. > They seem to have 64-bit time_t and just botch it in struct stat which gl= ibc has > to convert from the broken kernel structure to a userspace-facing form. > Looking at the other files in: >=20 > https://sourceware.org/git/?p=3Dglibc.git;a=3Dtree;f=3Dsysdeps/unix/sysv/= linux/m > ips/bits;h=3Dc1f9a8df22be6d1c0eb62e20d1113a9f9a11f49a;hb=3DHEAD >=20 > they seem to use glibc's __time_t in kernel interfaces directly (e.g. > the sysvipc stuff) and from what I can tell the kernel expects a 64-bit > __kernel_time_t in those places, so hopefully this isn't as bad as I inti= tially > thought. >=20 > > > (due to sizeof long in 64bit). Thus all members of ' struct stat' > > > starting from st_uid are at different offset than what stat syscall > > > has returned. In case of MIPS32, we just need to adjust dev_t (being > > > long long) as described in __stat_fix. However in case of MIPS64 we > > > need to members from 'struct kernel_stat' to 'struct stat' > > > > Yes... >=20 > As long as the time_t definition issue is not in question, I think this i= s an okay > path to move forward. It might be nice to make nlink_t match whatever gli= bc > used for it though so that our user-facing struct and glibc's have matchi= ng > layout/offsets. >=20 > Sorry for all the confusion over this. >=20 > Rich