RE: mips n64 porting review

[Jaydeep Patil <Jaydeep.Patil@imgtec.com>]

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
> 
> Sorry for dropping the CC. More follow-up below:
> 
> 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 <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; };
> > > >> +
> > > >> +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;
> > > >> +}
> > > >
> > > >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).
> 
> I've done further reading of what glibc and the kernel do, and it's confusing.
> They seem to have 64-bit time_t and just botch it in struct stat which glibc has
> to convert from the broken kernel structure to a userspace-facing form.
> Looking at the other files in:
> 
> https://sourceware.org/git/?p=glibc.git;a=tree;f=sysdeps/unix/sysv/linux/m
> ips/bits;h=c1f9a8df22be6d1c0eb62e20d1113a9f9a11f49a;hb=HEAD
> 
> 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 intitially
> thought.
> 
> > > (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...
> 
> As long as the time_t definition issue is not in question, I think this is an okay
> path to move forward. It might be nice to make nlink_t match whatever glibc
> used for it though so that our user-facing struct and glibc's have matching
> layout/offsets.
> 
> Sorry for all the confusion over this.
> 
> Rich