[PATCH] y2038: Reorder placement of st_ino in struct __stat64_t64

[Adhemerval Zanella]

On 14/10/2020 19:00, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>> On 14/10/2020 12:56, Lukasz Majewski wrote:
>>> Hi Adhemerval,
>>>   
>>>> On 14/10/2020 11:14, Lukasz Majewski wrote:  
>>>>> Hi Adhemerval,
>>>>>     
>>>>>> On 14/10/2020 10:00, Lukasz Majewski wrote:    
>>>>>>> In the installed struct stat{64} the __ino64_t st_ino member is
>>>>>>> placed in the end. This patch moves it to the same position as
>>>>>>> in the aforementioned, exported structures as it allows less
>>>>>>> #ifdefs for __USE_TIME_BITS64 support use case.      
>>>>>>
>>>>>> Why exactly this is required? Afaik this is glibc defined type
>>>>>> not currently only used internally and all member accesses are
>>>>>> set directly (no use or memcpy or offsetof).  Is this related to
>>>>>> the issue you saw on the arm environment?     
>>>>>
>>>>> This is required to make the minimal changes to provide
>>>>> -D_TIME_SIZE=64 support for {f}stat{at} as in:
>>>>> https://github.com/lmajewski/y2038_glibc/commit/26aa2ac07246682a505d85dac1c269689964b79b
>>>>>    
>>>>
>>>> But the idea it to decouple y2038 stat from the generic stat which
>>>> support non-LFS and make it support solely LFS.  
>>>
>>> And this idea is correct.
>>>   
>>>> As Joseph has pointed
>>>> out, the __stat64_t64 should be architecture-independent and I
>>>> think it simplifies its definition to untie from struct_stat.h
>>>> (which has multiple definitions due historically each architecture 
>>>> to tie with the selected kernel ABI).  
>>>
>>> This is also correct assumption.
>>>
>>> Please find my explanation of the problem as I may not been
>>> correctly understood:
>>>
>>> For example the __fstat64_time64 defined in
>>> sysdeps/unix/sysv/linux/fstat64.c has following signature:
>>>
>>> int __fstat64_time64 (int fd, struct __stat64_t64 *buf)
>>>
>>> for:
>>> - __WORDSIZE==64 is struct __stat64_t64 is aliased to struct
>>> stat64,  
>>
>> Not __WORDSIZE==64 but rather XSTAT_IS_XSTAT64, since mips64 and
>> mips64-n32 provides non-LFS interfaces. For XSTAT_IS_XSTAT64 ABIs,
>> __stat64_t64 is also essentially stat and the {f,l}stat{at}64 is
>> aliased to {f,l}stat{at}.
> 
> Ok.
> 
>>
>>> - __WORDSIZE==32 (Non Y2038) uses __fstat64 with struct stat64
>>> - __WORDSIZE==32 (Y2038) will redirect fstat calls to
>>> __fstat64_time64 and as a result it will have to accept *buf which
>>> must be binary compatible with struct stat{64} and __stat64_t64.  
>>
>> Yes, fstat on __WORDSIZE==32 with XSTAT_IS_XSTAT64 will be an alias
>> to __fstat64_time64 (and this now assumed that any new 32-bit ABI
>> will have to only provide LFS interfaces and 64-bit time_t).
> 
> I do agree.
> 
>>
>> Also for such cases __stat64_t64 *won't* be exported, its struct stat
>> *already* has 64-bit time fields.
> 
> Yes, correct.
> 
>>
>>>
>>> By binary compatible I mean the layout and fields must be identical
>>> as passing struct stat{64} to __fstat64_time64 will force casting
>>> of the data pointed by *buf.  
>>
>> I think there is a confusion here, for currently y2038 enabled ABIs
>> you will have either the first situation or the second you described.
>>
>> For future y2038 ABIs (basically all 32-bit that still supports
>> 32-bit time_t) __stat64_t64 and stat64 do *not* require to be binary
>> compatible.
> 
> Hmm.... 
> 
>>
>> In this case you will have:
>>
>>   1. non-LFS {f,l}stat{64} that accepts 'struct stat'.
> 
> By LFS I guess you mean the -D_LARGEFILE64_SOURCE compilation switch
> (which is optional and enables struct stat64 support).
> 
>>   2. LFS with 32-bit time fields {f,l}stat{64} that accepts 'struct
>> stat64'. 
> 
> Is this scenario valid? For Y2038 support on ARM usage of
> -D_TIME_BITS=64 also mandatory implies -D_FILE_OFFSET_BITS=64

Yes, this scenario is still valid since we have not deprecated non-LFS
call.  Recall that no non-LFS 64-bit time is *not* supported.

> 
>> 3. LFS with 64-bit time fields {f,l}fstat64_t64 (or any other
>>      name for the exported ABI) with accepts 'struct __stat64_t64'.
> 
> Ok. This is the case on which I'm working on.
> 
>>
>> This is *different* than the current 32-bit y2038 enabled ABIs
>> (arc, riscv32) which does not need to support neither 1. nor 2..
> 
> Ok. I know. Let's keep it aside as they support LFS and Y2038 from the
> outset.
> 
>> For this architecture the exported ABI is similar to current majority
>> 64-bit architectures (modules mips64...), where they exports
>> {f,l}stat{at} with {f,l}stat{at}{64} being just aliases. 
> 
> Ok.
> 
>>
>> So for instance on ARM, to actually use the y2038 calls we will need
>> to export the __{f,l}stat{at}64_time64 along with the 'struct
>> __stat64_t64' *and* redirects the LFS calls {f,l}stat{at}64 calls to
>> _{f,l}stat{at}64_time64 for _TIME_BITS=64 
> 
> I think I'm following the above guidelines in my WIP patch, which adds
> support for -D_TIME_BITS=64.
> 
>> (along with redefine the
>> 'struct stat64' to 'struct __stat64_t64').
> 
> Please correct me if I'm wrong, but I shall be doable to:
> (in unix/sysv/linux/bits/struct_stat.h)
> 
> #ifdef __USE_TIME_BITS64
> # include <struct_stat_time64.h>
> # define stat __stat64_t64
> #else
> struct stat
>   {
>   ...
>   }
> #endif /* __USE_TIME_BITS64  */
> 
> 
> And the same pattern for struct stat64.

What I think what needs to be done is:

  1. Move sysdeps/unix/sysv/linux/struct_stat_time64.h to
     sysdeps/unix/sysv/linux/bits/struct_stat_time64.h and adds it on
     sysdep_headeps at sysdeps/unix/sysv/linux/Makefile.  This will instruct to
     install the header.

  2. Either on sysdeps/unix/sysv/linux/bits/stat.h or 
     unix/sysv/linux/bits/struct_stat.h include the struct_stat_time64.h
     for __USE_TIME_BITS64.  I do not have strong preference here, but I think
     adding of unix/sysv/linux/bits/struct_stat.h results in less nesting and
     simplify things a bit.

  3. On io/sys/stat.h add the logic to redirect to the y2038 implementations:

---
#ifndef __USE_FILE_OFFSET64
/* Get file attributes for FILE and put them in BUF.  */
extern int stat (const char *__restrict __file,
                 struct stat *__restrict __buf) __THROW __nonnull ((1, 2));

/* Get file attributes for the file, device, pipe, or socket
   that file descriptor FD is open on and put them in BUF.  */
extern int fstat (int __fd, struct stat *__buf) __THROW __nonnull ((2));
#else
# ifdef __USE_TIME_BITS64
#   ifdef __REDIRECT_NTH
extern int __REDIRECT_NTH (stat, (const char *__restrict __file,
                                  struct __stat64_t64 *__restrict __buf), __stat64_t64)
     __nonnull ((1, 2));
extern int __REDIRECT_NTH (fstat, (int __fd, struct __stat64_t64 *__buf), __fstat64_t64)
     __nonnull ((2));
#   else
#    define stat64 __stat64_t64
#    define fstat64 __fstat64_t64
#   endif /* __REDIRECT_NTH  */
# else
#  ifdef __REDIRECT_NTH
extern int __REDIRECT_NTH (stat, (const char *__restrict __file,
                                  struct stat *__restrict __buf), stat64)
     __nonnull ((1, 2));
extern int __REDIRECT_NTH (fstat, (int __fd, struct stat *__buf), fstat64)
     __nonnull ((2));
#  else
#   define stat stat64
#   define fstat fstat64
#  endif /* __REDIRECT_NTH  */
# endif /* __USE_TIME_BITS64  */
#endif
#ifdef __USE_LARGEFILE64
# ifdef __USE_TIME_BITS64
extern int __REDIRECT_NTH (stat64, (const char *__restrict __file,
                                    struct __stat64_t64 *__restrict __buf), __stat64_t64)
     __nonnull ((1, 2));
extern int __REDIRECT_NTH (fstat64, (int __fd, struct __stat64_t64 *__buf), __fstat64_t64)
     __nonnull ((2));
# else
extern int stat64 (const char *__restrict __file,
                   struct stat64 *__restrict __buf) __THROW __nonnull ((1, 2));
extern int fstat64 (int __fd, struct stat64 *__buf) __THROW __nonnull ((2));
# endif /* __USE_TIME_BITS64  */
#endif
---

So y2038 safe interfaces will be used only if both __USE_FILE_OFFSET64 and
__USE_TIME_BITS64 are defined. What I am not sure if we should also need to 
export the y2038 interfaces for _LARGEFILE64_SOURCE.

I haven't tested it, so you would want to check if something is missing for
64-bit architectures with current 64-bit time support to see if it misses 
something.  We need to handle it to *not* redirect to the __{f}stat64_t64
interfaces, so maybe it should be a more complex ifdef definitions.