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

[Joseph Myers <joseph at codesourcery dot com>]

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.  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.

* What layout struct stat has for the generic ABI.  This is only an issue 
for a handful of headers for that ABI, so any macros defined for it - 
which must be in the implementation namespace - may not need to be defined 
at all for other ports.

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.

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.)

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.)

-- 
Joseph S. Myers
joseph@codesourcery.com