| Message ID | 1470304959-9944-3-git-send-email-ynorov@caviumnetworks.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.)
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.
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
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
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.
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
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.
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
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.
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
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.
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, 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 >
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.
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
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.
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
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.
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
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 \
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