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

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 9 Aug 2016, Yury Norov wrote:

> > * 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? 

This is about the patch you proposed that would have introduced incorrect 
uses of XSTAT_IS_XSTAT64 in installed 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.
> 
> 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.

No.  The following principles apply:

* The glibc API is architecture-independent without strong reasons an API 
needs to be architecture-specific.  Thus, _FILE_OFFSET_BITS is an API 
supported on all architectures (although on some architecture it doesn't 
actually change type sizes).  Likewise, fields of struct stat that have 
type off_t have so on all architectures and for all values of 
_FILE_OFFSET_BITS, although the type underlying that typedef name may 
vary.

* APIs specified by POSIX follow POSIX.  Thus, again, a field specified to 
have type off_t has that type, not off64_t, although sometimes those may 
be the same time.  Similarly, tv_nsec has type long.

* Installed headers must be namespace-clean.

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

There are at least three separate principles at work here.

* Installed headers must be namespace-clean, as above.

* For new macros, #if conditionals are preferred to #ifdef.

* glibc must be -Wundef-clean, both in the build of glibc itself and in 
the installed headers.  Note that the build of glibc itself will produce 
errors for any -Wundef warnings.

These do *not* imply that if a macro is tested somewhere in #if, it must 
be defined for all ports.  They only imply it must be defined for all 
ports *where the test of the macro gets used* (whether in the glibc build 
itself or in an installed header).  Existing ports that do not use the 
generic syscall ABI aren't going to start using it in future.

As for real bugs caught by -Wundef: see the ABI issues where macros 
relating to certain symbol versions didn't get defined properly, which 
were caught when we turned on -Wundef.

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

We're talking about *new ports*.  We can take the time to get the ABI 
right for them.

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

If for some reason the padding cannot be *inside* struct timespec, that 
needs to be explained at greater length.

-- 
Joseph S. Myers
joseph@codesourcery.com