Re: [PATCH v4 2/3] 32-bit ABIs: support stat syscall family

[Yury Norov <ynorov at caviumnetworks dot com>]

On Fri, Aug 05, 2016 at 05:28:38PM +0000, Joseph Myers wrote:
> On Fri, 5 Aug 2016, Yury Norov wrote:
> 
> > > As has been explained to you several times, XSTAT_IS_XSTAT64 is in the 
> > > user's namespace and must not be referenced in any installed header.
> > 
> > Then I have to introduce new settings in stat.h and statfs.h like
> > __STAT_MATCHES_STAT64 and __STATFS_MATCHES_STATFS64. The problem is
> > that there are too much non-generic stat{,fs} headers in glibc, and
> > I have to propagate new option to this ports: x86, alpha, powerpc,
> > sparc, s390, mips, ia64, m68k and microblaze.
> 
> It's far from clear that you need to do that.
> 
> The following discusses struct stat; statfs may well be similar (as patch 
> submitter, producing an analysis of the issues in sufficient depth is your 
> responsibility; this message just deals with one example issue in the sort 
> of depth required).  There are at least two separate issues:
> 
> * Whether certain pairs of functions should be aliased.  Right now this is 
> handled in some cases by XSTAT_IS_XSTAT64 in kernel_stat.h, but in other 
> cases, for example, by sysdeps/unix/sysv/linux/wordsize-64 having xstat.c 
> (which defines __xstat64 aliases) alongside empty xstat64.c.  It might 
> well be possible to use this macro more consistently and refactor the code 
> to reduce the number of source files for the implementation of these 
> functions.  It would also be desirable to move such a macro to the newer 
> typo-proof conventions (#if instead of #ifdef, always defined to 1 or 0).  
> In any case, this is purely information for the implementation, not for 
> the headers.

This is not about aarch64/ilp32 because XSTAT_IS_XSTAT64 is 64-bit
implementation option, correct? 

> The headers always declare stat and stat64 as separate 
> structures, which may or may not have the same layout, and always declare 
> separate functions for the types, which may or may not alias.  
> _FILE_OFFSET_BITS=64 adjusts the stat layout (to match stat64, but it's 
> still a separate type) and remaps function calls.

I think that having XSTAT_IS_XSTAT64 for implementation and
_FILE_OFFSET_BITS for interface is the bad idea for maintenance.
It's better to have a single option.

> * What layout struct stat has for the generic ABI.  This is only an issue 
> for a handful of headers for that ABI,

If struct stat has natural layout for the specific kernel, we can avoid using
compat layer on kernel side and on glibc side, which is of course better, and
costs nothing for runtime except few extra-bytes consumption. So if there's no 
other downsides, it's better to have it the same. And this is important for
generic code because this is the first option for new ports, and it
should be optimal.

> so any macros defined for it - 
> which must be in the implementation namespace - may not need to be defined 
> at all for other ports.

I lost my understanding of this rule. You and Andreas, and other
people told several times that #if/#ifdef is the coding style issue - 
typo-proof convention. (By the way, are there real examples of typos
of this sort passed thru review?) For me it means that any new code
should follow this rule. The only exception - when undefined macro
generates reasonable compile time error, like 32-bit macro used in
64-bit code.

Now I have to introduce new 32-bit option, and you tell me it may not
be defined for 10 32-bit ports. Some of them may use custom headers
with generic implementation, now or in future, so new option should
be defined for them too if we want to use #if everywhere.

> Your patch would adjust the __field64 definition in that bits/stat.h 
> header, in the XSTAT_IS_XSTAT64 case, so it defines struct stat fields to 
> have the types they would have if _FILE_OFFSET_BITS=64.  But if this 
> actually makes any difference, it is also the wrong thing to do.  For 
> example, your change would cause st_ino to have type __ino64_t rather than 
> __ino_t.  But st_ino must have type ino_t, and unless 
> _FILE_OFFSET_BITS=64, ino_t is a typedef for __ino_t, not __ino64_t.  So 
> actually you must arrange bits/typesizes.h so that in this case it defines 
> __INO_T_TYPE to have the appropriate type (same as __INO64_T_TYPE), at 
> which point the element of struct stat will automatically be right, 
> without needing any change to stat.h.

I was thinking about it too. The idea of new option (say, __STAT_MATCHES_STAT64)
is to tell the glibc that all fields are of identical size to 64-bit
version, and so there is no difference between ino_t and ino64_t as
they are identical too. If it's important though, I can do like this:

#if defined (__USE_FILE_OFFSET64) || 
# define __field64(type, type64, name) type64 name
#elif __STAT_MATCHES_STAT64
# define __field64(type, type64, name) type name
#else
# define __field64(type, type64, name) __type3264 (type, name)
#endif

(I already did it in internal branch)

> So maybe actually you want a different version of bits/typesizes.h for 
> newer generic-ABI ports, possibly in a sysdeps subdirectory for such 
> ports.  Or maybe some implementation-namespace macro that affects the 
> generic one (and isn't needed at all for non-generic-ABI ports).  (Or an 
> architecture-specific bits/typesizes.h such as you seem to be modifying in 
> one of your patches, though if the generic one can be made more generic, 
> that may avoid duplication.)

This is always the option to introduce new generic ABI, and it might
be better in some aspects. But this is much more work, and there are
people who are doing it (RISCV). And I'd like to listen what they
think. AFAIR there is no complete RISCV series to write aarch64/ilp32
on top of it. 

Right now I can create sysdeps/unix/sysv/linux/riscv/bits directory and 
place needed headers there. But I'd like to get an agreement on this,
and collect thoughts how to do it better.

> As for struct timespec inside struct stat: are you sure that special 
> handling in the struct stat declaration is the right thing to do, rather 
> than arranging for this port to define struct timespec to contain the 
> padding members and be appropriately aligned (with the kernel made to 
> ignore the high parts when struct timespec is passed to the kernel from an 
> ILP32 process, since those high parts are considered padding in 
> userspace)?  Putting the padding in struct timespec so the layout is 
> genuinely compatible between userspace and the kernel would seem a lot 
> less intrusive in glibc.  (Because of the pre-__USE_XOPEN2K8 case in 
> bits/stat.h, I suppose you'd need an implementation-namespace macro or two 
> somewhere for defining time_t and nanoseconds fields, that macro 
> automatically declaring the padding field as needed, and that macro then 
> being used in bits/stat.h as well as in the original definition of struct 
> timespec.)

It was my initial idea, but Arnd told that we cannot modify time_t and
struct timespec as it is used in some ioctls and this change may harm
compatibility.

I suggested to declare new time64_t and struct timespec64 and use it
in union with 32-bit versions where possible but it was rejected too.
So with 32-bit time_t/timespec, this is the only option to treat time
fields as special case and introduce a bunch of paddings.

Yury.