[v4,2/3] 32-bit ABIs: support stat syscall family
diff mbox

Message ID 1470304959-9944-3-git-send-email-ynorov@caviumnetworks.com
State New
Headers show

Commit Message

Yury Norov Aug. 4, 2016, 10:02 a.m. UTC
In modern APIs stat and statfs structures has their layouts identical
to 64-bit version after changing off_t, ino_t etc sizes to 64-bit.
It means we can pass it to kernel same way as 64-bit ABI does.

In this patch:
 - __ASSUME_SUPPORT_64_BIT_TIME_TYPES macro introduced to indicate that 32-bit
   ABI has struct __timespec (and maybe __timeval in future) that is compatible
   to 64-bit kernel timespec;
 - conv_timespec() and stat_conv_timespecs() macros are introduced to convert
   kernel 64-bit timespec to 32-bit timespec;
 - XSTAT_IS_XSTAT64 is reused in 32-bit code to hint GLIBC that structures  stat
   and statfs are identical to 64-bit versions, and 64-bit syscalls should be used
   for corresponding requests.
 - 32-bit syscalls are redirected to 64-bit version.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 sysdeps/unix/sysv/linux/fstatfs64.c                |  7 ++++
 sysdeps/unix/sysv/linux/fxstat64.c                 | 11 +++++
 sysdeps/unix/sysv/linux/fxstatat64.c               | 15 ++++++-
 sysdeps/unix/sysv/linux/generic/bits/stat.h        | 49 ++++++++++++++++++----
 sysdeps/unix/sysv/linux/generic/bits/statfs.h      | 32 ++++++++------
 .../unix/sysv/linux/generic/wordsize-32/fstatfs.c  |  3 ++
 .../unix/sysv/linux/generic/wordsize-32/fxstat.c   |  3 ++
 .../unix/sysv/linux/generic/wordsize-32/fxstatat.c |  2 +
 .../unix/sysv/linux/generic/wordsize-32/lxstat.c   |  2 +
 .../unix/sysv/linux/generic/wordsize-32/lxstat64.c | 20 ++++++++-
 .../unix/sysv/linux/generic/wordsize-32/statfs.c   |  2 +
 .../unix/sysv/linux/generic/wordsize-32/xstat.c    |  2 +
 .../unix/sysv/linux/generic/wordsize-32/xstat64.c  | 16 ++++++-
 sysdeps/unix/sysv/linux/kernel-time.h              | 45 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/statfs64.c                 |  8 ++++
 time/time.h                                        | 12 +++++-
 16 files changed, 204 insertions(+), 25 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/kernel-time.h

Comments

Joseph Myers Aug. 4, 2016, 12:40 p.m. UTC | #1
On Thu, 4 Aug 2016, Yury Norov wrote:

> diff --git a/sysdeps/unix/sysv/linux/generic/bits/stat.h b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> index 8e3f745..85e9866 100644
> --- a/sysdeps/unix/sysv/linux/generic/bits/stat.h
> +++ b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> @@ -41,7 +41,7 @@
>  /* Versions of the `xmknod' interface.  */
>  #define _MKNOD_VER_LINUX	0
>  
> -#if defined __USE_FILE_OFFSET64
> +#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)

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.

> +# if SUPPORT_64BIT_TIME_TYPES

Likewise SUPPORT_64BIT_TIME_TYPES.

Please stop posting any more patches to libc-alpha until you have done a 
thorough review of all the previous libc-alpha discussions of AArch64 
ILP32 patches over the past few years and properly understood the issues 
involved and properly addressed them in your patches.  By now, your patch 
postings are just wasting people's time, and this is not acceptable; 
there's no point seeking review if you keep ignoring the issues raised.

> +#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)

Likewise.

> +#ifdef XSTAT_IS_XSTAT64

Likewise.

> --- a/time/time.h
> +++ b/time/time.h
> @@ -65,6 +65,17 @@ __USING_NAMESPACE_STD(clock_t)
>  #endif /* clock_t not defined and <time.h> or need clock_t.  */
>  #undef	__need_clock_t
>  
> +#ifndef SUPPORT_64BIT_TIME_TYPES

Likewise.
Yury Norov Aug. 5, 2016, 8:54 a.m. UTC | #2
On Thu, Aug 04, 2016 at 12:40:29PM +0000, Joseph Myers wrote:
> On Thu, 4 Aug 2016, Yury Norov wrote:
> 
> > diff --git a/sysdeps/unix/sysv/linux/generic/bits/stat.h b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> > index 8e3f745..85e9866 100644
> > --- a/sysdeps/unix/sysv/linux/generic/bits/stat.h
> > +++ b/sysdeps/unix/sysv/linux/generic/bits/stat.h
> > @@ -41,7 +41,7 @@
> >  /* Versions of the `xmknod' interface.  */
> >  #define _MKNOD_VER_LINUX	0
> >  
> > -#if defined __USE_FILE_OFFSET64
> > +#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)
> 
> 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.

With all that, I have to introduce 3 new options under generic
sysdeps/unix/sysv/linux and dozen of platforms , 
and add another portion of conditional compilation directives to
generic code which of course does not increase readability.

I see 2 ways to avoid it:
 - introduce new riscv directory and place new code there as it is
   going to be common for all new 32-bit ports, or
 - move aarch64/ilp32 code back under platform directory as it was
   initially done.

1st option is looking too far from my direct work, but I can try to do
it if it seems like better choice. So my questions are:

 - Is everything else except namespace issues is OK?
 - should we change something in generic approach like I told above?
 - I'm worrying about __type3264() usage in case of struct timespec.
   It looks little hacky, and the code will not work if struct timespec
   (doubtedly) get changed one day, so 32-bit version will not be 2 times
   smaller than 64-bit one.
 - There's an option to introduce more generic macro for padded fields, like:
   #define __padded(type, name, npads) type name; char __##name##_pad[npads]
   (plus alignement, 32/64 and BE/LE checks).
   So __field64() will look like this:
   #define __field64(type, type64, name) __padded (type, name, sizeof (type))
   Does it make sense, or __type3264 is enough?

> > +# if SUPPORT_64BIT_TIME_TYPES
> 
> Likewise SUPPORT_64BIT_TIME_TYPES.
> 
> Please stop posting any more patches to libc-alpha until you have done a 
> thorough review of all the previous libc-alpha discussions of AArch64 
> ILP32 patches over the past few years and properly understood the issues 
> involved and properly addressed them in your patches.  By now, your patch 
> postings are just wasting people's time, and this is not acceptable; 
> there's no point seeking review if you keep ignoring the issues raised.
> 
> > +#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)
> 
> Likewise.
> 
> > +#ifdef XSTAT_IS_XSTAT64
> 
> Likewise.
> 
> > --- a/time/time.h
> > +++ b/time/time.h
> > @@ -65,6 +65,17 @@ __USING_NAMESPACE_STD(clock_t)
> >  #endif /* clock_t not defined and <time.h> or need clock_t.  */
> >  #undef	__need_clock_t
> >  
> > +#ifndef SUPPORT_64BIT_TIME_TYPES
> 
> Likewise.
> 
> -- 
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers Aug. 5, 2016, 5:28 p.m. UTC | #3
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.)
Yury Norov Aug. 9, 2016, 1:26 p.m. UTC | #4
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.
Arnd Bergmann Aug. 9, 2016, 2:23 p.m. UTC | #5
On Tuesday, August 9, 2016 4:26:45 PM CEST Yury Norov wrote:
> > 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.

Right. The discussion for the y2038 glibc port has progressed a bit,
and we will in fact see a way to use 64-bit glibc time_t with a kernel
that has 32-bit time_t for backwards compatibility reasons, but this
is very far from being at the point where a glibc port can make that
the only option.
 
> 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.

Let's take a step back here. We had a very long discussion about the
kernel ABI, and in particular the definition of 'struct stat64' for
aarch64-ilp32. To recap, there were three options (we probably had
an implementation for each one at some point):

a) use the generic definition for 'stat64' from the architecture
   independent headers: This has the downside of requiring extra
   syscall entry points in the kernel for the new ABI, as the layout
   is different from what 32-bit ARM uses, and our normal compat
   handlers provide those.

b) use the definition from 32-bit ARM, and share the compat stat64
   syscalls: This has the downside of using an awkward layout that
   has the STAT64_HAS_BROKEN_ST_INO set, and has two copies of
   st_ino.

c) Use the layout that is compatible with 64-bit ARM (aarch64) kernels:
   The downside here is that it does not use the standard types for
   the time stamps, so you end up having to handle it in an architecture
   specific way in user space.

We ended up with approach c), which seemed the most future-proof
ABI, but if the decision was to not use the generic ABI, why do you
try to add this to the generic implementation? The easiest way
would be to just have a aarch64 specific conversion function that
copies the data from the kernel structure into the generic user
space structure.

Alternatively you can add a sysdeps/unix/sysv/linux/aarch64/bits/stat.h
file that defines the structure the same way that the kernel does, but
you don't need to come up with a generic way of doing this, as (almost
certainly) no other architecture will have this problem, and we will have
to solve the more general 64-bit time_t problem independently.

Joseph or Andreas can probably say which of those ways for handling
this in aarch64 specific code is better (override bits/stat.h or
convert the structure), and we can also go back to approach b)
above (using the 32-bit ARM compatible structure) if that ends
up being simpler.

	Arnd
Yury Norov Aug. 9, 2016, 3:15 p.m. UTC | #6
On Tue, Aug 09, 2016 at 04:23:27PM +0200, Arnd Bergmann wrote:
> On Tuesday, August 9, 2016 4:26:45 PM CEST Yury Norov wrote:
> > > 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.
> 
> Right. The discussion for the y2038 glibc port has progressed a bit,
> and we will in fact see a way to use 64-bit glibc time_t with a kernel
> that has 32-bit time_t for backwards compatibility reasons, but this
> is very far from being at the point where a glibc port can make that
> the only option.
>  

Could you share this discussion to me?

> > 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.
> 
> Let's take a step back here. We had a very long discussion about the
> kernel ABI, and in particular the definition of 'struct stat64' for
> aarch64-ilp32. To recap, there were three options (we probably had
> an implementation for each one at some point):
> 
> a) use the generic definition for 'stat64' from the architecture
>    independent headers: This has the downside of requiring extra
>    syscall entry points in the kernel for the new ABI, as the layout
>    is different from what 32-bit ARM uses, and our normal compat
>    handlers provide those.
> 
> b) use the definition from 32-bit ARM, and share the compat stat64
>    syscalls: This has the downside of using an awkward layout that
>    has the STAT64_HAS_BROKEN_ST_INO set, and has two copies of
>    st_ino.
> 
> c) Use the layout that is compatible with 64-bit ARM (aarch64) kernels:
>    The downside here is that it does not use the standard types for
>    the time stamps, so you end up having to handle it in an architecture
>    specific way in user space.
> 
> We ended up with approach c), which seemed the most future-proof
> ABI, but if the decision was to not use the generic ABI, why do you
> try to add this to the generic implementation? The easiest way
> would be to just have a aarch64 specific conversion function that
> copies the data from the kernel structure into the generic user
> space structure.
> 
> Alternatively you can add a sysdeps/unix/sysv/linux/aarch64/bits/stat.h
> file that defines the structure the same way that the kernel does, but
> you don't need to come up with a generic way of doing this, as (almost
> certainly) no other architecture will have this problem, and we will have
> to solve the more general 64-bit time_t problem independently.

This is how it was implemented initially. But Joseph requested to
re-use generic glibc code as much as possible. In fact, if we use
64-bit off_t etc, the only difference is struct timespec, and there
are 3 options we should consider to handle it:
 1. Add pads to generic structure (current).
 2. Declare stat structures in arch code (initial).
 3. Introduce riscv generic ABI - what I'm asking about here.

struct stat{,fs} layout for aarch64/ilp32 is going to be the new
standard for 32-bit ports, so I think we'd end up with 1 or 3, and
that's what Joseph tells about, if I understand him right.

> Joseph or Andreas can probably say which of those ways for handling
> this in aarch64 specific code is better (override bits/stat.h or
> convert the structure), and we can also go back to approach b)
> above (using the 32-bit ARM compatible structure) if that ends
> up being simpler.
> 
> 	Arnd
Andreas Schwab Aug. 9, 2016, 3:53 p.m. UTC | #7
On Di, Aug 09 2016, Yury Norov <ynorov@caviumnetworks.com> wrote:

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

These are independent concepts.  The first is about the kernel interface
and internal to glibc, the second is a user option.

Andreas.
Arnd Bergmann Aug. 9, 2016, 3:59 p.m. UTC | #8
On Tuesday, August 9, 2016 6:15:43 PM CEST Yury Norov wrote:
> On Tue, Aug 09, 2016 at 04:23:27PM +0200, Arnd Bergmann wrote:
> > On Tuesday, August 9, 2016 4:26:45 PM CEST Yury Norov wrote:
> > > > 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.
> > 
> > Right. The discussion for the y2038 glibc port has progressed a bit,
> > and we will in fact see a way to use 64-bit glibc time_t with a kernel
> > that has 32-bit time_t for backwards compatibility reasons, but this
> > is very far from being at the point where a glibc port can make that
> > the only option.
> >  
> 
> Could you share this discussion to me?

See the thread "Fourth draft of the Y2038 design document"

> > Alternatively you can add a sysdeps/unix/sysv/linux/aarch64/bits/stat.h
> > file that defines the structure the same way that the kernel does, but
> > you don't need to come up with a generic way of doing this, as (almost
> > certainly) no other architecture will have this problem, and we will have
> > to solve the more general 64-bit time_t problem independently.
> 
> This is how it was implemented initially. But Joseph requested to
> re-use generic glibc code as much as possible. In fact, if we use
> 64-bit off_t etc, the only difference is struct timespec, and there
> are 3 options we should consider to handle it:
>  1. Add pads to generic structure (current).
>  2. Declare stat structures in arch code (initial).
>  3. Introduce riscv generic ABI - what I'm asking about here.

Ok, I see. I would not change anything for riscv *here*, because
riscv does not have the issue of having to be compatible with
a nonstandard 'struct stat64' in one of two 32-bit user space
ABIs.

> struct stat{,fs} layout for aarch64/ilp32 is going to be the new
> standard for 32-bit ports, so I think we'd end up with 1 or 3, and
> that's what Joseph tells about, if I understand him right.

For statfs certainly: we want to use a single structure definition
for any new architectures, including aarch64-ilp32 and riscv32.

For stat, there are two distinct issues, and it's possible that
I also forgot about the second one in previous emails:

1. We want to have a generic structure that is used on riscv32
   and on future architectures that all share the common 'struct
   stat64' layout from the Linux kernel, and that should absolutely
   use 64-bit ino_t and off_t.

2. Because of what we decided for the kernel ABI, aarch64/ilp32
   actually uses a nonstandard structure definition that is
   uses 64-bit time_t unlike any other architecture. This port
   is unique here because we have to deal with 32-bit ARM support
   in the kernel and we don't want to have yet another set of
   syscall handlers for the stat family.

The second point is where it gets interesting, and I agree it's
not as clear-cut as I hoped it would be. I can see a multitude
of ways to deal with this, and to make this worse, most of them
require agreement from both kernel and glibc folks regarding how
we want to handle this in the future.

Let me try to describe what I can see as possible ways forward:

a) use the normal 'struct stat' definition that we get with
   _FILE_OFFSET_BITS=64 and with 32-bit time_t on aarch64-ilp32,
   and convert the kernel structure to that in an aarch64
   specific helper in glibc. This is probably the easiest way.

b) revise 'struct stat64' in the kernel to use 64-bit timestamps
   for all future architectures, and get glibc to expose
   that to applications. This is basically what you are doing
   now, except with buy-in from the kernel side. The main
   reason we haven't done this already is that there has
   been talk about a new 'xstat' syscall interface with
   a much-expanded ABI for multiple years now, and it still
   looks like that's coming any time soon.

c) finally get 'xstat' into the kernel, and add a new
   stat implementation based on that for glibc, obsoleting
   any architecture differences. This would also help with
   the y2038 effort (at least on the kernel side).

d) go back to your previous approach of defining 'struct stat'
   as aarch64-ilp32 specific, acknowledging that we made a
   mistake in the assumption that it would be the new generic
   format.

e) go further back to defining the aarch64-ilp32 stat structure
   to be identical with the 32-bit arm structure and change
   the kernel ABI to use that too. This would avoid the need
   for any conversion, and the need for defining padded
   timespec structures, but also get us back to
   STAT64_HAS_BROKEN_ST_INO.

	Arnd
Yury Norov Aug. 9, 2016, 5:40 p.m. UTC | #9
On Tue, Aug 09, 2016 at 05:59:24PM +0200, Arnd Bergmann wrote:
> sYcomyljbmeKCvvhv5G/JU0dNbNWxw==
> MIME-Version: 1.0
> Status: O
> Content-Length: 5693
> Lines: 114
> 
> On Tuesday, August 9, 2016 6:15:43 PM CEST Yury Norov wrote:
> > On Tue, Aug 09, 2016 at 04:23:27PM +0200, Arnd Bergmann wrote:
> > > On Tuesday, August 9, 2016 4:26:45 PM CEST Yury Norov wrote:
> > > > > 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.
> > > 
> > > Right. The discussion for the y2038 glibc port has progressed a bit,
> > > and we will in fact see a way to use 64-bit glibc time_t with a kernel
> > > that has 32-bit time_t for backwards compatibility reasons, but this
> > > is very far from being at the point where a glibc port can make that
> > > the only option.
> > >  
> > 
> > Could you share this discussion to me?
> 
> See the thread "Fourth draft of the Y2038 design document"


Thanks

> > > Alternatively you can add a sysdeps/unix/sysv/linux/aarch64/bits/stat.h
> > > file that defines the structure the same way that the kernel does, but
> > > you don't need to come up with a generic way of doing this, as (almost
> > > certainly) no other architecture will have this problem, and we will have
> > > to solve the more general 64-bit time_t problem independently.
> > 
> > This is how it was implemented initially. But Joseph requested to
> > re-use generic glibc code as much as possible. In fact, if we use
> > 64-bit off_t etc, the only difference is struct timespec, and there
> > are 3 options we should consider to handle it:
> >  1. Add pads to generic structure (current).
> >  2. Declare stat structures in arch code (initial).
> >  3. Introduce riscv generic ABI - what I'm asking about here.
> 
> Ok, I see. I would not change anything for riscv *here*, because
> riscv does not have the issue of having to be compatible with
> a nonstandard 'struct stat64' in one of two 32-bit user space
> ABIs.
> 
> > struct stat{,fs} layout for aarch64/ilp32 is going to be the new
> > standard for 32-bit ports, so I think we'd end up with 1 or 3, and
> > that's what Joseph tells about, if I understand him right.
> 
> For statfs certainly: we want to use a single structure definition
> for any new architectures, including aarch64-ilp32 and riscv32.
> 
> For stat, there are two distinct issues, and it's possible that
> I also forgot about the second one in previous emails:
> 
> 1. We want to have a generic structure that is used on riscv32
>    and on future architectures that all share the common 'struct
>    stat64' layout from the Linux kernel, and that should absolutely
>    use 64-bit ino_t and off_t.
> 
> 2. Because of what we decided for the kernel ABI, aarch64/ilp32
>    actually uses a nonstandard structure definition that is
>    uses 64-bit time_t unlike any other architecture. This port
>    is unique here because we have to deal with 32-bit ARM support
>    in the kernel and we don't want to have yet another set of
>    syscall handlers for the stat family.

Arnd, I don't understand it. Right now we use standard unistd.h in
kernel which assumes we use standard structs stat and statfs. Correct?

And in glibc we take stat{,fs} layouts from standard
sysdeps/unix/sysv/linux/generic/bits (though we add few macros, but
nobody complains). So what non-standard definitions are you meaning?


For time_t aarch64/ilp32 uses 32-bit type like any other 32-bit port.
To avoid introducing another set of handlers/conversion layers, I
added pads in userspace which lets us to pass ilp32 syscalls to
lp64 handlers w/o compat conversions.

> The second point is where it gets interesting, and I agree it's
> not as clear-cut as I hoped it would be. I can see a multitude
> of ways to deal with this, and to make this worse, most of them
> require agreement from both kernel and glibc folks regarding how
> we want to handle this in the future.
> 
> Let me try to describe what I can see as possible ways forward:
> 
> a) use the normal 'struct stat' definition that we get with
>    _FILE_OFFSET_BITS=64 and with 32-bit time_t on aarch64-ilp32,
>    and convert the kernel structure to that in an aarch64
>    specific helper in glibc. This is probably the easiest way.
> 
> b) revise 'struct stat64' in the kernel to use 64-bit timestamps
>    for all future architectures, and get glibc to expose
>    that to applications.

I don't expose 64-bit timestamps. I leave pads, so kernel fills it
with 64-bit data. But then I use conv_timespec() to convert
them to 32-bit representations. So 64-bit time exists in glibc
internals only and not exposed to applications.

>    This is basically what you are doing
>    now, except with buy-in from the kernel side. The main
>    reason we haven't done this already is that there has
>    been talk about a new 'xstat' syscall interface with
>    a much-expanded ABI for multiple years now, and it still
>    looks like that's coming any time soon.
> 
> c) finally get 'xstat' into the kernel, and add a new
>    stat implementation based on that for glibc, obsoleting
>    any architecture differences. This would also help with
>    the y2038 effort (at least on the kernel side).

I didn't read xstat thread. For 64-bit time only, we don't need
new interface I, think.

> d) go back to your previous approach of defining 'struct stat'
>    as aarch64-ilp32 specific, acknowledging that we made a
>    mistake in the assumption that it would be the new generic
>    format.
> 
> e) go further back to defining the aarch64-ilp32 stat structure
>    to be identical with the 32-bit arm structure and change
>    the kernel ABI to use that too. This would avoid the need
>    for any conversion, and the need for defini

e) is what I really don't like. Aarch32 is something really
non-standard. So if we choose between non-standard 32-bit ABI and
standard 64-bit one, and both of them are already exist, I'd choose
64-bit of course.
Arnd Bergmann Aug. 9, 2016, 8:16 p.m. UTC | #10
On Tuesday, August 9, 2016 8:40:44 PM CEST Yury Norov wrote:
> > 
> > For stat, there are two distinct issues, and it's possible that
> > I also forgot about the second one in previous emails:
> > 
> > 1. We want to have a generic structure that is used on riscv32
> >    and on future architectures that all share the common 'struct
> >    stat64' layout from the Linux kernel, and that should absolutely
> >    use 64-bit ino_t and off_t.
> > 
> > 2. Because of what we decided for the kernel ABI, aarch64/ilp32
> >    actually uses a nonstandard structure definition that is
> >    uses 64-bit time_t unlike any other architecture. This port
> >    is unique here because we have to deal with 32-bit ARM support
> >    in the kernel and we don't want to have yet another set of
> >    syscall handlers for the stat family.
> 
> Arnd, I don't understand it. Right now we use standard unistd.h in
> kernel which assumes we use standard structs stat and statfs. Correct?

We do? That would be a bug, because the kernel by default uses
the defintion of 'stat64' from arch/arm64/include/asm/stat.h,
which is what arch/arm/ uses for its native syscall, and which
is slightly different from the structure definition in 
include/uapi/asm-generic/stat.h.

For all I know, we had decided to use the binary layout of
'struct stat' that 64-bit machines use, and that is *not* what
the standard unistd.h expects.

> And in glibc we take stat{,fs} layouts from standard
> sysdeps/unix/sysv/linux/generic/bits (though we add few macros, but
> nobody complains). So what non-standard definitions are you meaning?

I looked up the details again now:

The binary layout is almost identical on big-endian builds,
except the ARM structure does not support 64-bit st_ino numbers
in the second 64-bit field, but rather has them in the last
64-bit field at the end of the structure, as indicated by the
'STAT64_HAS_BROKEN_ST_INO' macro.

On little-endian machines, st_ino is more broken, as only the
upper half of the 64-bit field is set by the kernel.

I was actually expecting larger differences, so I guess we could
decide to just set STAT64_HAS_BROKEN_ST_INO here and be done
with it.

> For time_t aarch64/ilp32 uses 32-bit type like any other 32-bit port.
> To avoid introducing another set of handlers/conversion layers, I
> added pads in userspace which lets us to pass ilp32 syscalls to
> lp64 handlers w/o compat conversions.

Ok, this matches with my understanding, in my previous email
I was just explaining why we use the lp64 handlers for 'stat',
as that normally makes no sense here.

> > The second point is where it gets interesting, and I agree it's
> > not as clear-cut as I hoped it would be. I can see a multitude
> > of ways to deal with this, and to make this worse, most of them
> > require agreement from both kernel and glibc folks regarding how
> > we want to handle this in the future.
> > 
> > Let me try to describe what I can see as possible ways forward:
> > 
> > a) use the normal 'struct stat' definition that we get with
> >    _FILE_OFFSET_BITS=64 and with 32-bit time_t on aarch64-ilp32,
> >    and convert the kernel structure to that in an aarch64
> >    specific helper in glibc. This is probably the easiest way.
> > 
> > b) revise 'struct stat64' in the kernel to use 64-bit timestamps
> >    for all future architectures, and get glibc to expose
> >    that to applications.
> 
> I don't expose 64-bit timestamps. I leave pads, so kernel fills it
> with 64-bit data. But then I use conv_timespec() to convert
> them to 32-bit representations. So 64-bit time exists in glibc
> internals only and not exposed to applications.
> 
> >    This is basically what you are doing
> >    now, except with buy-in from the kernel side. The main
> >    reason we haven't done this already is that there has
> >    been talk about a new 'xstat' syscall interface with
> >    a much-expanded ABI for multiple years now, and it still
> >    looks like that's coming any time soon.
> > 
> > c) finally get 'xstat' into the kernel, and add a new
> >    stat implementation based on that for glibc, obsoleting
> >    any architecture differences. This would also help with
> >    the y2038 effort (at least on the kernel side).
> 
> I didn't read xstat thread. For 64-bit time only, we don't need
> new interface I, think.

See https://lwn.net/Articles/685519/ for a description of
what xstat does, it is useful for a number of reasons, one
of them being 64-bit timestamps.

> > d) go back to your previous approach of defining 'struct stat'
> >    as aarch64-ilp32 specific, acknowledging that we made a
> >    mistake in the assumption that it would be the new generic
> >    format.
> > 
> > e) go further back to defining the aarch64-ilp32 stat structure
> >    to be identical with the 32-bit arm structure and change
> >    the kernel ABI to use that too. This would avoid the need
> >    for any conversion, and the need for defini
> 
> e) is what I really don't like. Aarch32 is something really
> non-standard. So if we choose between non-standard 32-bit ABI and
> standard 64-bit one, and both of them are already exist, I'd choose
> 64-bit of course.

Actually if STAT64_HAS_BROKEN_ST_INO is the only incompatibility,
I don't really see a problem with that. The only downside of
this is that we don't have any remaining 64-bit padding fields that
we could use for extending 'struct stat64', but that is true for
many other architectures, and 'xstat' will solve the problem.

	Arnd
Joseph Myers Aug. 9, 2016, 8:29 p.m. UTC | #11
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.
Arnd Bergmann Aug. 9, 2016, 8:58 p.m. UTC | #12
On Tuesday, August 9, 2016 8:29:13 PM CEST Joseph Myers wrote:
> > 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.

We cannot define 'struct timespec' to be incompatible with the version
used by the kernel, that would break all other interfaces passing a
timespec.

While in theory the kernel could change the definition of its "compat"
data types for 32-bit tasks on 64-bit kernels (as it does for x86/x32),
we are not doing that for aarch64, in order to stay compatible with
device drivers that already have working compat mode on 32-bit ARM.

	Arnd
Joseph Myers Aug. 9, 2016, 9:03 p.m. UTC | #13
On Tue, 9 Aug 2016, Arnd Bergmann wrote:

> We cannot define 'struct timespec' to be incompatible with the version
> used by the kernel, that would break all other interfaces passing a
> timespec.
> 
> While in theory the kernel could change the definition of its "compat"
> data types for 32-bit tasks on 64-bit kernels (as it does for x86/x32),
> we are not doing that for aarch64, in order to stay compatible with
> device drivers that already have working compat mode on 32-bit ARM.

So in that case, if the kernel's struct stat uses 64-bit timespec, it's 
inevitably different from the userspace timespec.  Which leaves me 
wondering what the advantages of a userspace layout that's almost but not 
quite the same as the kernel's layout (and so needs timespec fields 
rearranged on structs received from the kernel) are over not doing 
anything special about the timespec fields in the userspace struct 
definition (that is, no special padding / alignment) and instead using the 
existing machinery for converting struct stat to convert from the kernel 
structure to the userspace structure.
Arnd Bergmann Aug. 9, 2016, 9:17 p.m. UTC | #14
On Tuesday, August 9, 2016 9:03:54 PM CEST Joseph Myers wrote:
> On Tue, 9 Aug 2016, Arnd Bergmann wrote:
> 
> > We cannot define 'struct timespec' to be incompatible with the version
> > used by the kernel, that would break all other interfaces passing a
> > timespec.
> > 
> > While in theory the kernel could change the definition of its "compat"
> > data types for 32-bit tasks on 64-bit kernels (as it does for x86/x32),
> > we are not doing that for aarch64, in order to stay compatible with
> > device drivers that already have working compat mode on 32-bit ARM.
> 
> So in that case, if the kernel's struct stat uses 64-bit timespec, it's 
> inevitably different from the userspace timespec.  Which leaves me 
> wondering what the advantages of a userspace layout that's almost but not 
> quite the same as the kernel's layout (and so needs timespec fields 
> rearranged on structs received from the kernel) are over not doing 
> anything special about the timespec fields in the userspace struct 
> definition (that is, no special padding / alignment) and instead using the 
> existing machinery for converting struct stat to convert from the kernel 
> structure to the userspace structure.

Right, that would be what I listed as approach a) in my mail with
message id 1914420.rMinRNpl2s@wuerfel earlier today, i.e. the
easiest way to solve the problem.

	Arnd
Bamvor Jian Zhang Aug. 10, 2016, 12:44 p.m. UTC | #15
Hi,

On 2016/8/9 22:23, Arnd Bergmann wrote:
> On Tuesday, August 9, 2016 4:26:45 PM CEST Yury Norov wrote:
>>> 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.
>
> Right. The discussion for the y2038 glibc port has progressed a bit,
> and we will in fact see a way to use 64-bit glibc time_t with a kernel
> that has 32-bit time_t for backwards compatibility reasons, but this
> is very far from being at the point where a glibc port can make that
> the only option.
>
>> 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.
>
> Let's take a step back here. We had a very long discussion about the
> kernel ABI, and in particular the definition of 'struct stat64' for
> aarch64-ilp32. To recap, there were three options (we probably had
> an implementation for each one at some point):
>
> a) use the generic definition for 'stat64' from the architecture
>     independent headers: This has the downside of requiring extra
>     syscall entry points in the kernel for the new ABI, as the layout
>     is different from what 32-bit ARM uses, and our normal compat
>     handlers provide those.
>
> b) use the definition from 32-bit ARM, and share the compat stat64
>     syscalls: This has the downside of using an awkward layout that
>     has the STAT64_HAS_BROKEN_ST_INO set, and has two copies of
>     st_ino.
>
> c) Use the layout that is compatible with 64-bit ARM (aarch64) kernels:
>     The downside here is that it does not use the standard types for
>     the time stamps, so you end up having to handle it in an architecture
>     specific way in user space.

> We ended up with approach c), which seemed the most future-proof
> ABI, but if the decision was to not use the generic ABI, why do you
> try to add this to the generic implementation? The easiest way
> would be to just have a aarch64 specific conversion function that
> copies the data from the kernel structure into the generic user
> space structure.
>
> Alternatively you can add a sysdeps/unix/sysv/linux/aarch64/bits/stat.h
> file that defines the structure the same way that the kernel does, but
> you don't need to come up with a generic way of doing this, as (almost
> certainly) no other architecture will have this problem, and we will have
> to solve the more general 64-bit time_t problem independently.
>
> Joseph or Andreas can probably say which of those ways for handling
> this in aarch64 specific code is better (override bits/stat.h or
> convert the structure), and we can also go back to approach b)
> above (using the 32-bit ARM compatible structure) if that ends
> up being simpler.
Compare with b and c. IIUC, the difference is where we convert the stat/statfs.
b convert in kernel while c convert in glibc. I wanted to test the performance
between these two approaches. But after read the code, I found that kernel
always convert stat/statfs, it means that b is faster than c. If we chose b, we
need to define the stat64 in aarch64/bit/stat.h. Any reason why we could not do
it?

> 	Arnd
>
Yury Norov Aug. 17, 2016, 7:13 p.m. UTC | #16
On Tue, Aug 09, 2016 at 11:17:59PM +0200, Arnd Bergmann wrote:
> On Tuesday, August 9, 2016 9:03:54 PM CEST Joseph Myers wrote:
> > On Tue, 9 Aug 2016, Arnd Bergmann wrote:
> > 
> > > We cannot define 'struct timespec' to be incompatible with the version
> > > used by the kernel, that would break all other interfaces passing a
> > > timespec.
> > > 
> > > While in theory the kernel could change the definition of its "compat"
> > > data types for 32-bit tasks on 64-bit kernels (as it does for x86/x32),
> > > we are not doing that for aarch64, in order to stay compatible with
> > > device drivers that already have working compat mode on 32-bit ARM.
> > 
> > So in that case, if the kernel's struct stat uses 64-bit timespec, it's 
> > inevitably different from the userspace timespec.  Which leaves me 
> > wondering what the advantages of a userspace layout that's almost but not 
> > quite the same as the kernel's layout (and so needs timespec fields 
> > rearranged on structs received from the kernel) are over not doing 
> > anything special about the timespec fields in the userspace struct 
> > definition (that is, no special padding / alignment) and instead using the 
> > existing machinery for converting struct stat to convert from the kernel 
> > structure to the userspace structure.
> 
> Right, that would be what I listed as approach a) in my mail with
> message id 1914420.rMinRNpl2s@wuerfel earlier today, i.e. the
> easiest way to solve the problem.
> 
> 	Arnd

Hi Arnd, Joseph,

Is my understanding correct that you suggest to use __xstat_conv(),
like in sysdeps/unix/sysv/linux/fxstat.c? I was thinking on it. If
aarch64/ilp32 was the single case, I'd choose it. But this (64-bit
fields) is new standard, so any other who will add new port will meet
this problems again, and will add similar hacks to kernel_stat.h and
xstatconv.h as I (like defining __NR_fstat to __NR_fstat64). 

So I did choose new layout prior to existing conversion because:
 - this is generic behavior and so generic solution is preferable.
 - as it is generic, it should be effective, and useless copying of
   kernel_stat fields to stat fields and extra stack consumption are
   not welcome.
 - 32-bit time_t is not for too long, and one day kernel and userspace
   layouts of struct stat will become completely identical, so any
   conversion will not be needed anymore.
 - and therefore differences in time fields may be treated as special
   case, and I can manipulate them special way.

After this discussion I realize that some of my assumptions may be
wrong. But I still think that new layout is better choice than the
runtime conversion of almost identical structures.

Anyway, I'm ready to rewrite it. Just give me clear understanding what
we want.

Yury.
Arnd Bergmann Aug. 25, 2016, 3:34 p.m. UTC | #17
On Wednesday, August 17, 2016 10:13:57 PM CEST Yury Norov wrote:
> Hi Arnd, Joseph,
> 
> Is my understanding correct that you suggest to use __xstat_conv(),
> like in sysdeps/unix/sysv/linux/fxstat.c? I was thinking on it. If
> aarch64/ilp32 was the single case, I'd choose it. But this (64-bit
> fields) is new standard, so any other who will add new port will meet
> this problems again, and will add similar hacks to kernel_stat.h and
> xstatconv.h as I (like defining __NR_fstat to __NR_fstat64). 
> 
> So I did choose new layout prior to existing conversion because:
>  - this is generic behavior and so generic solution is preferable.
>  - as it is generic, it should be effective, and useless copying of
>    kernel_stat fields to stat fields and extra stack consumption are
>    not welcome.
>  - 32-bit time_t is not for too long, and one day kernel and userspace
>    layouts of struct stat will become completely identical, so any
>    conversion will not be needed anymore.
>  - and therefore differences in time fields may be treated as special
>    case, and I can manipulate them special way.
> 
> After this discussion I realize that some of my assumptions may be
> wrong. But I still think that new layout is better choice than the
> runtime conversion of almost identical structures.
> 
> Anyway, I'm ready to rewrite it. Just give me clear understanding what
> we want.

Here is my (current) take on the situation:

The kernel interface for 'stat' on new 32-bit architectures is what
is defined in 'struct stat64' in include/uapi/asm-generic/stat.h.

This structure uses 32-bit time fields, whether we like it or not.
When we get a generic replacement for 'struct stat64' on 32-bit
architectures and the 'sys_stat64' implementation in the kernel,
that structure will use 64-bit time members, but have a different name
and will be available on all architectures (no arch specific
definitions for new structures), so don't worry about 64-bit
time_t here.

We've gone back and forth on the arm64+ilp32 definition of the
existing stat64 , because that one cannot easily use the same
generic structure. I originally thought that using the 64-bit
layout of 'struct stat' was a good idea for arm64, but now
I don't see much of an advantage either way (the existing arm32
'struct stat64' layout or the existing  arm64 'struct stat' layout).

Both of them are incompatible with the default layout for all
other new 32-bit architectures in user space, so pick one of the
two for the kernel interface, and convert it to the normal layout
in glibc using __xstat_conv():

- if you use the arm64 'struct stat' layout (as in the current
  kernel patches), you need a temporary buffer with the larger
  size and then copy over the time fields one by one
- if you instead use the arm32 'struct stat64' layout in the kernel,
  the size is the same, and the conversion is a one-line
  assignment of st_ino.

	Arnd
Yury Norov Aug. 29, 2016, 4:01 p.m. UTC | #18
On Thu, Aug 25, 2016 at 05:34:23PM +0200, Arnd Bergmann wrote:
> On Wednesday, August 17, 2016 10:13:57 PM CEST Yury Norov wrote:
> > Hi Arnd, Joseph,
> > 
> > Is my understanding correct that you suggest to use __xstat_conv(),
> > like in sysdeps/unix/sysv/linux/fxstat.c? I was thinking on it. If
> > aarch64/ilp32 was the single case, I'd choose it. But this (64-bit
> > fields) is new standard, so any other who will add new port will meet
> > this problems again, and will add similar hacks to kernel_stat.h and
> > xstatconv.h as I (like defining __NR_fstat to __NR_fstat64). 
> > 
> > So I did choose new layout prior to existing conversion because:
> >  - this is generic behavior and so generic solution is preferable.
> >  - as it is generic, it should be effective, and useless copying of
> >    kernel_stat fields to stat fields and extra stack consumption are
> >    not welcome.
> >  - 32-bit time_t is not for too long, and one day kernel and userspace
> >    layouts of struct stat will become completely identical, so any
> >    conversion will not be needed anymore.
> >  - and therefore differences in time fields may be treated as special
> >    case, and I can manipulate them special way.
> > 
> > After this discussion I realize that some of my assumptions may be
> > wrong. But I still think that new layout is better choice than the
> > runtime conversion of almost identical structures.
> > 
> > Anyway, I'm ready to rewrite it. Just give me clear understanding what
> > we want.
> 
> Here is my (current) take on the situation:
> 
> The kernel interface for 'stat' on new 32-bit architectures is what
> is defined in 'struct stat64' in include/uapi/asm-generic/stat.h.
> 
> This structure uses 32-bit time fields, whether we like it or not.
> When we get a generic replacement for 'struct stat64' on 32-bit
> architectures and the 'sys_stat64' implementation in the kernel,
> that structure will use 64-bit time members, but have a different name
> and will be available on all architectures (no arch specific
> definitions for new structures), so don't worry about 64-bit
> time_t here.
> 
> We've gone back and forth on the arm64+ilp32 definition of the
> existing stat64 , because that one cannot easily use the same
> generic structure. I originally thought that using the 64-bit
> layout of 'struct stat' was a good idea for arm64, but now
> I don't see much of an advantage either way (the existing arm32
> 'struct stat64' layout or the existing  arm64 'struct stat' layout).
> 
> Both of them are incompatible with the default layout for all
> other new 32-bit architectures in user space, so pick one of the
> two for the kernel interface, and convert it to the normal layout
> in glibc using __xstat_conv():
> 
> - if you use the arm64 'struct stat' layout (as in the current
>   kernel patches), you need a temporary buffer with the larger
>   size and then copy over the time fields one by one
> - if you instead use the arm32 'struct stat64' layout in the kernel,
>   the size is the same, and the conversion is a one-line
>   assignment of st_ino.
> 
> 	Arnd

Ok. I'll do like this. I'd prefer 1st option, because 2nd implies
allocating big buffer on kernel side.

Yury.
Arnd Bergmann Aug. 29, 2016, 8:34 p.m. UTC | #19
On Monday 29 August 2016, Yury Norov wrote:
> > 
> > We've gone back and forth on the arm64+ilp32 definition of the
> > existing stat64 , because that one cannot easily use the same
> > generic structure. I originally thought that using the 64-bit
> > layout of 'struct stat' was a good idea for arm64, but now
> > I don't see much of an advantage either way (the existing arm32
> > 'struct stat64' layout or the existing  arm64 'struct stat' layout).
> > 
> > Both of them are incompatible with the default layout for all
> > other new 32-bit architectures in user space, so pick one of the
> > two for the kernel interface, and convert it to the normal layout
> > in glibc using __xstat_conv():
> > 
> > - if you use the arm64 'struct stat' layout (as in the current
> >   kernel patches), you need a temporary buffer with the larger
> >   size and then copy over the time fields one by one
> > - if you instead use the arm32 'struct stat64' layout in the kernel,
> >   the size is the same, and the conversion is a one-line
> >   assignment of st_ino.
> > 
> >       Arnd
> 
> Ok. I'll do like this. I'd prefer 1st option, because 2nd implies
> allocating big buffer on kernel side.

On the kernel side, both require allocating a buffer, the second
one is just slightly bigger. The kernel uses its own internal
'struct kstat', which gets copied into one of the other structures.

	Arnd
Yury Norov Aug. 30, 2016, 11:31 a.m. UTC | #20
On Mon, Aug 29, 2016 at 10:34:38PM +0200, Arnd Bergmann wrote:
> On Monday 29 August 2016, Yury Norov wrote:
> > > 
> > > We've gone back and forth on the arm64+ilp32 definition of the
> > > existing stat64 , because that one cannot easily use the same
> > > generic structure. I originally thought that using the 64-bit
> > > layout of 'struct stat' was a good idea for arm64, but now
> > > I don't see much of an advantage either way (the existing arm32
> > > 'struct stat64' layout or the existing  arm64 'struct stat' layout).
> > > 
> > > Both of them are incompatible with the default layout for all
> > > other new 32-bit architectures in user space, so pick one of the
> > > two for the kernel interface, and convert it to the normal layout
> > > in glibc using __xstat_conv():
> > > 
> > > - if you use the arm64 'struct stat' layout (as in the current
> > >   kernel patches), you need a temporary buffer with the larger
> > >   size and then copy over the time fields one by one
> > > - if you instead use the arm32 'struct stat64' layout in the kernel,
> > >   the size is the same, and the conversion is a one-line
> > >   assignment of st_ino.
> > > 
> > >       Arnd
> > 
> > Ok. I'll do like this. I'd prefer 1st option, because 2nd implies
> > allocating big buffer on kernel side.
> 
> On the kernel side, both require allocating a buffer, the second
> one is just slightly bigger. The kernel uses its own internal
> 'struct kstat', which gets copied into one of the other structures.
> 
> 	Arnd

Ah, and in kernel too...

Anyway, using the arm32 'struct stat64' cannot help because
__xstat{,32,64}_conv() assumes copying of the kernel buffer to user
buffer.

We can, of course, write custom __xstat_conv() to avoid that copying. 
But then we'd write custom syscalls for stat family for aarch64/ilp32.
More simple and straightforward way then is to introduce custom layout
for struct stat, and avoid all conversions and memory wasting at all.
And this is what I suggested in the very first series of ilp32 support.
But Joseph declined it as non-generic solution.

So for me there are 3 options:
 - __xstat_conv() - hacky, complex and ineffective. I work on it right
   now, and I'm not happy.
 - current version with conversion of time fields only - adds new
   fresh portion of hacks, but little more effective.
 - initial version. Effective, simple, free of hacks but (because of)
   non-generic.

Stat-related code in glibc is probably the worst code in open source
I ever seen. That's why I don't want ilp32 to use it. But if it's
impossible, I'd end up with aarch64. At least it doesn't have broken
fields.
Arnd Bergmann Aug. 30, 2016, 3:15 p.m. UTC | #21
On Tuesday 30 August 2016, Yury Norov wrote:
> On Mon, Aug 29, 2016 at 10:34:38PM +0200, Arnd Bergmann wrote:
> > On Monday 29 August 2016, Yury Norov wrote:
> > > > 
> > > > We've gone back and forth on the arm64+ilp32 definition of the
> > > > existing stat64 , because that one cannot easily use the same
> > > > generic structure. I originally thought that using the 64-bit
> > > > layout of 'struct stat' was a good idea for arm64, but now
> > > > I don't see much of an advantage either way (the existing arm32
> > > > 'struct stat64' layout or the existing  arm64 'struct stat' layout).
> > > > 
> > > > Both of them are incompatible with the default layout for all
> > > > other new 32-bit architectures in user space, so pick one of the
> > > > two for the kernel interface, and convert it to the normal layout
> > > > in glibc using __xstat_conv():
> > > > 
> > > > - if you use the arm64 'struct stat' layout (as in the current
> > > >   kernel patches), you need a temporary buffer with the larger
> > > >   size and then copy over the time fields one by one
> > > > - if you instead use the arm32 'struct stat64' layout in the kernel,
> > > >   the size is the same, and the conversion is a one-line
> > > >   assignment of st_ino.
> > > > 
> > > >       Arnd
> > > 
> > > Ok. I'll do like this. I'd prefer 1st option, because 2nd implies
> > > allocating big buffer on kernel side.
> > 
> > On the kernel side, both require allocating a buffer, the second
> > one is just slightly bigger. The kernel uses its own internal
> > 'struct kstat', which gets copied into one of the other structures.
> > 
> > 	Arnd
> 
> Ah, and in kernel too...
> 
> Anyway, using the arm32 'struct stat64' cannot help because
> __xstat{,32,64}_conv() assumes copying of the kernel buffer to user
> buffer.
> 
> We can, of course, write custom __xstat_conv() to avoid that copying. 

Right, that would work. No idea if that would be preferred in glibc
or not: the existing __xstat_conv() obviously works already, while
the simpler replacement would mean extra code.

> But then we'd write custom syscalls for stat family for aarch64/ilp32.
> More simple and straightforward way then is to introduce custom layout
> for struct stat, and avoid all conversions and memory wasting at all.
> And this is what I suggested in the very first series of ilp32 support.
> But Joseph declined it as non-generic solution.

Ok. 

> So for me there are 3 options:
>  - __xstat_conv() - hacky, complex and ineffective. I work on it right
>    now, and I'm not happy.
>  - current version with conversion of time fields only - adds new
>    fresh portion of hacks, but little more effective.

+ new version with conversion of st_ino field only, a different new hack
  but simpler.

>  - initial version. Effective, simple, free of hacks but (because of)
>    non-generic.

I can't find that version now, can you post a copy of that original
patch or a link to an archived discussion?

The layout we want is exactly what we have for stat64
sysdeps/unix/sysv/linux/bits/stat.h (not
sysdeps/unix/sysv/linux/generic/bits/stat.h), right?

	Arnd

Patch
diff mbox

diff --git a/sysdeps/unix/sysv/linux/fstatfs64.c b/sysdeps/unix/sysv/linux/fstatfs64.c
index a624de6..e0297c4 100644
--- a/sysdeps/unix/sysv/linux/fstatfs64.c
+++ b/sysdeps/unix/sysv/linux/fstatfs64.c
@@ -15,6 +15,8 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
+#define __fstatfs __statfs_disable
+#define fstatfs statfs_disable
 
 #include <errno.h>
 #include <string.h>
@@ -70,3 +72,8 @@  __fstatfs64 (int fd, struct statfs64 *buf)
 #endif
 }
 weak_alias (__fstatfs64, fstatfs64)
+
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__fstatfs64, __fstatfs)
+weak_alias (__fstatfs64, fstatfs)
+#endif
diff --git a/sysdeps/unix/sysv/linux/fxstat64.c b/sysdeps/unix/sysv/linux/fxstat64.c
index 5468dd6..0a65fa2 100644
--- a/sysdeps/unix/sysv/linux/fxstat64.c
+++ b/sysdeps/unix/sysv/linux/fxstat64.c
@@ -15,6 +15,7 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
+#define __fxstat __fxstat_disable
 
 #include <errno.h>
 #include <stddef.h>
@@ -25,6 +26,7 @@ 
 #include <sys/syscall.h>
 
 #include <kernel-features.h>
+#include <kernel-time.h>
 
 /* Get information about the file FD in BUF.  */
 
@@ -36,12 +38,16 @@  ___fxstat64 (int vers, int fd, struct stat64 *buf)
 #if defined _HAVE_STAT64___ST_INO && !defined __ASSUME_ST_INO_64_BIT
   if (__builtin_expect (!result, 1) && buf->__st_ino != (__ino_t) buf->st_ino)
     buf->st_ino = buf->__st_ino;
+  return result;
 #endif
+  if (!result)
+    stat_conv_timespecs (buf);
   return result;
 }
 
 #include <shlib-compat.h>
 
+#undef __fxstat
 #if SHLIB_COMPAT(libc, GLIBC_2_1, GLIBC_2_2)
 versioned_symbol (libc, ___fxstat64, __fxstat64, GLIBC_2_2);
 strong_alias (___fxstat64, __old__fxstat64)
@@ -51,3 +57,8 @@  hidden_ver (___fxstat64, __fxstat64)
 strong_alias (___fxstat64, __fxstat64)
 hidden_def (__fxstat64)
 #endif
+
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__fxstat64, __fxstat)
+libc_hidden_ver (__fxstat64, __fxstat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/fxstatat64.c b/sysdeps/unix/sysv/linux/fxstatat64.c
index 7ffa2d4..0346d7c 100644
--- a/sysdeps/unix/sysv/linux/fxstatat64.c
+++ b/sysdeps/unix/sysv/linux/fxstatat64.c
@@ -14,6 +14,7 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
+#define __fxstatat __fxstatat_disable
 
 #include <errno.h>
 #include <fcntl.h>
@@ -26,6 +27,8 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#include <kernel-time.h>
+
 /* Get information about the file NAME in BUF.  */
 
 int
@@ -39,9 +42,19 @@  __fxstatat64 (int vers, int fd, const char *file, struct stat64 *st, int flag)
 
   result = INTERNAL_SYSCALL (fstatat64, err, 4, fd, file, st, flag);
   if (!__builtin_expect (INTERNAL_SYSCALL_ERROR_P (result, err), 1))
-    return 0;
+    {
+      stat_conv_timespecs (st);
+      return 0;
+    }
   else
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (INTERNAL_SYSCALL_ERRNO (result,
 								      err));
 }
 libc_hidden_def (__fxstatat64)
+
+#undef __fxstatat
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__fxstatat64, __fxstatat)
+libc_hidden_ver (__fxstatat64, __fxstatat)
+#endif
+
diff --git a/sysdeps/unix/sysv/linux/generic/bits/stat.h b/sysdeps/unix/sysv/linux/generic/bits/stat.h
index 8e3f745..85e9866 100644
--- a/sysdeps/unix/sysv/linux/generic/bits/stat.h
+++ b/sysdeps/unix/sysv/linux/generic/bits/stat.h
@@ -41,7 +41,7 @@ 
 /* Versions of the `xmknod' interface.  */
 #define _MKNOD_VER_LINUX	0
 
-#if defined __USE_FILE_OFFSET64
+#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)
 # define __field64(type, type64, name) type64 name
 #else
 # define __field64(type, type64, name) __type3264 (type, name)
@@ -68,19 +68,37 @@  struct stat
        identifier 'timespec' to appear in the <sys/stat.h> header.
        Therefore we have to handle the use of this header in strictly
        standard-compliant sources special.  */
-    struct timespec st_atim;		/* Time of last access.  */
-    struct timespec st_mtim;		/* Time of last modification.  */
-    struct timespec st_ctim;		/* Time of last status change.  */
+
+# if SUPPORT_64BIT_TIME_TYPES
+    __type3264 (struct timespec, st_atim);		/* Time of last access.  */
+    __type3264 (struct timespec, st_mtim);		/* Time of last modification.  */
+    __type3264 (struct timespec, st_ctim);		/* Time of last status change.  */
+# else
+    struct timespec st_atim;				/* Time of last access.  */
+    struct timespec st_mtim;				/* Time of last modification.  */
+    struct timespec st_ctim;				/* Time of last status change.  */
+# endif
+
 # define st_atime st_atim.tv_sec	/* Backward compatibility.  */
 # define st_mtime st_mtim.tv_sec
 # define st_ctime st_ctim.tv_sec
+
 #else
+# if SUPPORT_64BIT_TIME_TYPES
+    __type3264 (__time_t, st_atime);			/* Time of last access.  */
+    __type3264 (unsigned long int, st_atimensec);	/* Nscecs of last access.  */
+    __type3264 (__time_t, st_mtime);			/* Time of last modification.  */
+    __type3264 (unsigned long int, st_mtimensec);	/* Nsecs of last modification.  */
+    __type3264 (__time_t, st_ctime);			/* Time of last status change.  */
+    __type3264 (unsigned long int, st_ctimensec);	/* Nsecs of last status change.  */
+# else
     __time_t st_atime;			/* Time of last access.  */
     unsigned long int st_atimensec;	/* Nscecs of last access.  */
     __time_t st_mtime;			/* Time of last modification.  */
     unsigned long int st_mtimensec;	/* Nsecs of last modification.  */
     __time_t st_ctime;			/* Time of last status change.  */
     unsigned long int st_ctimensec;	/* Nsecs of last status change.  */
+# endif
 #endif
     int __glibc_reserved[2];
   };
@@ -109,16 +127,33 @@  struct stat64
        identifier 'timespec' to appear in the <sys/stat.h> header.
        Therefore we have to handle the use of this header in strictly
        standard-compliant sources special.  */
-    struct timespec st_atim;		/* Time of last access.  */
-    struct timespec st_mtim;		/* Time of last modification.  */
-    struct timespec st_ctim;		/* Time of last status change.  */
+
+# if SUPPORT_64BIT_TIME_TYPES
+    __type3264 (struct timespec, st_atim);		/* Time of last access.  */
+    __type3264 (struct timespec, st_mtim);		/* Time of last modification.  */
+    __type3264 (struct timespec, st_ctim);		/* Time of last status change.  */
+# else
+    struct timespec st_atim;				/* Time of last access.  */
+    struct timespec st_mtim;				/* Time of last modification.  */
+    struct timespec st_ctim;				/* Time of last status change.  */
+# endif
+
 #else
+# if SUPPORT_64BIT_TIME_TYPES
+    __type3264 (__time_t, st_atime);			/* Time of last access.  */
+    __type3264 (unsigned long int, st_atimensec);	/* Nscecs of last access.  */
+    __type3264 (__time_t, st_mtime);			/* Time of last modification.  */
+    __type3264 (unsigned long int, st_mtimensec);	/* Nsecs of last modification.  */
+    __type3264 (__time_t, st_ctime);			/* Time of last status change.  */
+    __type3264 (unsigned long int, st_ctimensec);	/* Nsecs of last status change.  */
+# else
     __time_t st_atime;			/* Time of last access.  */
     unsigned long int st_atimensec;	/* Nscecs of last access.  */
     __time_t st_mtime;			/* Time of last modification.  */
     unsigned long int st_mtimensec;	/* Nsecs of last modification.  */
     __time_t st_ctime;			/* Time of last status change.  */
     unsigned long int st_ctimensec;	/* Nsecs of last status change.  */
+# endif
 #endif
     int __glibc_reserved[2];
   };
diff --git a/sysdeps/unix/sysv/linux/generic/bits/statfs.h b/sysdeps/unix/sysv/linux/generic/bits/statfs.h
index 96629ac..0245c73 100644
--- a/sysdeps/unix/sysv/linux/generic/bits/statfs.h
+++ b/sysdeps/unix/sysv/linux/generic/bits/statfs.h
@@ -32,26 +32,32 @@ 
    using __USE_FILE_OFFSET64 only see the low 32 bits of some
    of the fields (the __fsblkcnt_t and __fsfilcnt_t fields).  */
 
-#if defined __USE_FILE_OFFSET64
+#if defined (__USE_FILE_OFFSET64) || defined (XSTAT_IS_XSTAT64)
 # define __field64(type, type64, name) type64 name
 #else
 # define __field64(type, type64, name) __type3264 (type, name)
 #endif
 
+#ifdef XSTAT_IS_XSTAT64
+# define __statfs_word_t long long
+#else
+# define __statfs_word_t __SWORD_TYPE
+#endif
+
 struct statfs
   {
-    __SWORD_TYPE f_type;
-    __SWORD_TYPE f_bsize;
+    __statfs_word_t f_type;
+    __statfs_word_t f_bsize;
     __field64(__fsblkcnt_t, __fsblkcnt64_t, f_blocks);
     __field64(__fsblkcnt_t, __fsblkcnt64_t, f_bfree);
     __field64(__fsblkcnt_t, __fsblkcnt64_t, f_bavail);
     __field64(__fsfilcnt_t, __fsfilcnt64_t, f_files);
     __field64(__fsfilcnt_t, __fsfilcnt64_t, f_ffree);
     __fsid_t f_fsid;
-    __SWORD_TYPE f_namelen;
-    __SWORD_TYPE f_frsize;
-    __SWORD_TYPE f_flags;
-    __SWORD_TYPE f_spare[4];
+    __statfs_word_t f_namelen;
+    __statfs_word_t f_frsize;
+    __statfs_word_t f_flags;
+    __statfs_word_t f_spare[4];
   };
 
 #undef __field64
@@ -59,18 +65,18 @@  struct statfs
 #ifdef __USE_LARGEFILE64
 struct statfs64
   {
-    __SWORD_TYPE f_type;
-    __SWORD_TYPE f_bsize;
+    __statfs_word_t f_type;
+    __statfs_word_t f_bsize;
     __fsblkcnt64_t f_blocks;
     __fsblkcnt64_t f_bfree;
     __fsblkcnt64_t f_bavail;
     __fsfilcnt64_t f_files;
     __fsfilcnt64_t f_ffree;
     __fsid_t f_fsid;
-    __SWORD_TYPE f_namelen;
-    __SWORD_TYPE f_frsize;
-    __SWORD_TYPE f_flags;
-    __SWORD_TYPE f_spare[4];
+    __statfs_word_t f_namelen;
+    __statfs_word_t f_frsize;
+    __statfs_word_t f_flags;
+    __statfs_word_t f_spare[4];
   };
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/fstatfs.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/fstatfs.c
index be9599a..9f20377 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/fstatfs.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/fstatfs.c
@@ -20,6 +20,8 @@ 
 #include <sys/statfs.h>
 #include <stddef.h>
 
+#ifndef XSTAT_IS_XSTAT64
+
 #include "overflow.h"
 
 /* Return information about the filesystem on which FD resides.  */
@@ -30,3 +32,4 @@  __fstatfs (int fd, struct statfs *buf)
   return rc ?: statfs_overflow (buf);
 }
 weak_alias (__fstatfs, fstatfs)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstat.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstat.c
index dd52011..b246f5d 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstat.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstat.c
@@ -25,6 +25,7 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#ifndef XSTAT_IS_XSTAT64
 #include "overflow.h"
 
 /* Get information about the file FD in BUF.  */
@@ -43,3 +44,5 @@  __fxstat (int vers, int fd, struct stat *buf)
 
 hidden_def (__fxstat)
 weak_alias (__fxstat, _fxstat);
+#endif
+
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstatat.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstatat.c
index dc7f934..b00f65d 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstatat.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/fxstatat.c
@@ -26,6 +26,7 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#ifndef XSTAT_IS_XSTAT64
 #include "overflow.h"
 
 /* Get information about the file NAME in BUF.  */
@@ -42,3 +43,4 @@  __fxstatat (int vers, int fd, const char *file, struct stat *buf, int flag)
   return -1;
 }
 libc_hidden_def (__fxstatat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat.c
index 395f98b..4fec6c9 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat.c
@@ -25,6 +25,7 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#ifndef XSTAT_IS_XSTAT64
 #include "overflow.h"
 
 /* Get information about the file NAME in BUF.  */
@@ -41,3 +42,4 @@  __lxstat (int vers, const char *name, struct stat *buf)
   return -1;
 }
 hidden_def (__lxstat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat64.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat64.c
index e1c15a8..9dc06c9 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat64.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/lxstat64.c
@@ -15,6 +15,7 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
+#define __lxstat __lxstat_disable
 
 #include <errno.h>
 #include <stddef.h>
@@ -25,14 +26,29 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#include <kernel-time.h>
+
 /* Get information about the file NAME in BUF.  */
 int
 __lxstat64 (int vers, const char *name, struct stat64 *buf)
 {
   if (vers == _STAT_VER_KERNEL)
-    return INLINE_SYSCALL (fstatat64, 4, AT_FDCWD, name, buf,
-                           AT_SYMLINK_NOFOLLOW);
+    {
+      int rc = INLINE_SYSCALL (fstatat64, 4, AT_FDCWD, name, buf,
+			       AT_SYMLINK_NOFOLLOW);
+      if (!rc)
+	stat_conv_timespecs (buf);
+
+      return rc;
+    }
+
   errno = EINVAL;
   return -1;
 }
 hidden_def (__lxstat64)
+
+#undef __lxstat
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__lxstat64, __lxstat)
+hidden_ver (__lxstat64, __lxstat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/statfs.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/statfs.c
index 1937f05..1a09f60 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/statfs.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/statfs.c
@@ -20,6 +20,7 @@ 
 #include <sys/statfs.h>
 #include <stddef.h>
 
+#ifndef XSTAT_IS_XSTAT64
 #include "overflow.h"
 
 /* Return information about the filesystem on which FILE resides.  */
@@ -31,3 +32,4 @@  __statfs (const char *file, struct statfs *buf)
 }
 libc_hidden_def (__statfs)
 weak_alias (__statfs, statfs)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat.c
index fdd2cb0..8fc13bd 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat.c
@@ -25,6 +25,7 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#ifndef XSTAT_IS_XSTAT64
 #include "overflow.h"
 
 /* Get information about the file NAME in BUF.  */
@@ -41,3 +42,4 @@  __xstat (int vers, const char *name, struct stat *buf)
   return -1;
 }
 hidden_def (__xstat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat64.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat64.c
index 2252337..f8d15e5 100644
--- a/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat64.c
+++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/xstat64.c
@@ -15,6 +15,7 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
+#define __xstat __xstat_disable
 
 #include <errno.h>
 #include <stddef.h>
@@ -25,14 +26,27 @@ 
 #include <sysdep.h>
 #include <sys/syscall.h>
 
+#include <kernel-time.h>
+
 /* Get information about the file NAME in BUF.  */
 int
 __xstat64 (int vers, const char *name, struct stat64 *buf)
 {
   if (vers == _STAT_VER_KERNEL)
-    return INLINE_SYSCALL (fstatat64, 4, AT_FDCWD, name, buf, 0);
+    {
+      int rc = INLINE_SYSCALL (fstatat64, 4, AT_FDCWD, name, buf, 0);
+      if (!rc)
+	stat_conv_timespecs (buf);
+      return rc;
+    }
 
   errno = EINVAL;
   return -1;
 }
 hidden_def (__xstat64)
+
+#undef __xstat
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__xstat64, __xstat)
+hidden_ver (__xstat64, __xstat)
+#endif
diff --git a/sysdeps/unix/sysv/linux/kernel-time.h b/sysdeps/unix/sysv/linux/kernel-time.h
new file mode 100644
index 0000000..ba14cf5
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/kernel-time.h
@@ -0,0 +1,45 @@ 
+/* Helpers to convert kernel time structures to user-visible ones.
+
+   Copyright (C) 1991-2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#if SUPPORT_64BIT_TIME_TYPES
+struct __timespec {
+  long long tv_sec;
+  long long tv_nsec;
+};
+
+# define conv_timespec(ts)					\
+  do								\
+    {								\
+      struct __timespec  *__ts = (void *) (ts);			\
+      (ts)->tv_sec = __ts->tv_sec;				\
+      (ts)->tv_nsec = __ts->tv_nsec;				\
+    }								\
+  while (0)
+# define stat_conv_timespecs(__stat)				\
+  do								\
+    {								\
+      conv_timespec (&(__stat)->st_atim);			\
+      conv_timespec (&(__stat)->st_mtim);			\
+      conv_timespec (&(__stat)->st_ctim);			\
+    }								\
+  while (0)
+#else
+# define conv_timespec(ts, _ts)		do {} while (0)
+# define stat_conv_timespecs(__stat)	do {} while (0)
+#endif
diff --git a/sysdeps/unix/sysv/linux/statfs64.c b/sysdeps/unix/sysv/linux/statfs64.c
index de42261..2ab2fb1 100644
--- a/sysdeps/unix/sysv/linux/statfs64.c
+++ b/sysdeps/unix/sysv/linux/statfs64.c
@@ -15,6 +15,8 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
+#define __statfs __statfs_disable
+#define statfs statfs_disable
 
 #include <errno.h>
 #include <string.h>
@@ -72,3 +74,9 @@  __statfs64 (const char *file, struct statfs64 *buf)
 #endif
 }
 weak_alias (__statfs64, statfs64)
+
+#ifdef XSTAT_IS_XSTAT64
+strong_alias (__statfs64, __statfs)
+libc_hidden_ver (__statfs64, __statfs)
+weak_alias (__statfs64, statfs)
+#endif
diff --git a/time/time.h b/time/time.h
index cc93917..8b4cced 100644
--- a/time/time.h
+++ b/time/time.h
@@ -65,6 +65,17 @@  __USING_NAMESPACE_STD(clock_t)
 #endif /* clock_t not defined and <time.h> or need clock_t.  */
 #undef	__need_clock_t
 
+#ifndef SUPPORT_64BIT_TIME_TYPES
+# ifdef __ASSUME_SUPPORT_64_BIT_TIME_TYPES
+#  if __WORDSIZE != 32
+#   error "__ASSUME_SUPPORT_64_BIT_TIME_TYPES is 32-bit only option"
+#  endif
+#  define SUPPORT_64BIT_TIME_TYPES	1
+# else
+#  define SUPPORT_64BIT_TIME_TYPES	0
+# endif
+#endif
+
 #if !defined __time_t_defined && (defined _TIME_H || defined __need_time_t)
 # define __time_t_defined	1
 
@@ -105,7 +116,6 @@  typedef __timer_t timer_t;
 #endif /* timer_t not defined and <time.h> or need timer_t.  */
 #undef	__need_timer_t
 
-
 #if (!defined __timespec_defined					\
      && ((defined _TIME_H						\
 	  && (defined __USE_POSIX199309					\