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

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

[Lukasz Majewski]

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.
---
 sysdeps/unix/sysv/linux/struct_stat_time64.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/struct_stat_time64.h b/sysdeps/unix/sysv/linux/struct_stat_time64.h
index b85385b6f3..2c57f4e562 100644
--- a/sysdeps/unix/sysv/linux/struct_stat_time64.h
+++ b/sysdeps/unix/sysv/linux/struct_stat_time64.h
@@ -43,7 +43,6 @@
 struct __stat64_t64
   {
     __dev_t st_dev;			/* Device.  */
-    __ino64_t st_ino;			/* file serial number.	*/
     __mode_t st_mode;			/* File mode.  */
     __nlink_t st_nlink;			/* Link count.  */
     __uid_t st_uid;			/* User ID of the file's owner.	*/
@@ -70,6 +69,7 @@ struct __stat64_t64
     __fieldts64 (st_mtime);
     __fieldts64 (st_ctime);
 #   endif /* __USE_XOPEN2K8  */
+    __ino64_t st_ino;			/* file serial number.	*/
   };
 
 #   define _STATBUF_ST_BLKSIZE
-- 
2.20.1

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

[Adhemerval Zanella]

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? 

> ---
>  sysdeps/unix/sysv/linux/struct_stat_time64.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/struct_stat_time64.h b/sysdeps/unix/sysv/linux/struct_stat_time64.h
> index b85385b6f3..2c57f4e562 100644
> --- a/sysdeps/unix/sysv/linux/struct_stat_time64.h
> +++ b/sysdeps/unix/sysv/linux/struct_stat_time64.h
> @@ -43,7 +43,6 @@
>  struct __stat64_t64
>    {
>      __dev_t st_dev;			/* Device.  */
> -    __ino64_t st_ino;			/* file serial number.	*/
>      __mode_t st_mode;			/* File mode.  */
>      __nlink_t st_nlink;			/* Link count.  */
>      __uid_t st_uid;			/* User ID of the file's owner.	*/
> @@ -70,6 +69,7 @@ struct __stat64_t64
>      __fieldts64 (st_mtime);
>      __fieldts64 (st_ctime);
>  #   endif /* __USE_XOPEN2K8  */
> +    __ino64_t st_ino;			/* file serial number.	*/
>    };
>  
>  #   define _STATBUF_ST_BLKSIZE
> 

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]

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

> 
> > ---
> >  sysdeps/unix/sysv/linux/struct_stat_time64.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/struct_stat_time64.h
> > b/sysdeps/unix/sysv/linux/struct_stat_time64.h index
> > b85385b6f3..2c57f4e562 100644 ---
> > a/sysdeps/unix/sysv/linux/struct_stat_time64.h +++
> > b/sysdeps/unix/sysv/linux/struct_stat_time64.h @@ -43,7 +43,6 @@
> >  struct __stat64_t64
> >    {
> >      __dev_t st_dev;			/* Device.  */
> > -    __ino64_t st_ino;			/* file serial
> > number.	*/ __mode_t st_mode;			/* File
> > mode.  */ __nlink_t st_nlink;			/* Link count.
> >  */ __uid_t st_uid;			/* User ID of the file's
> > owner.	*/ @@ -70,6 +69,7 @@ struct __stat64_t64
> >      __fieldts64 (st_mtime);
> >      __fieldts64 (st_ctime);
> >  #   endif /* __USE_XOPEN2K8  */
> > +    __ino64_t st_ino;			/* file serial
> > number.	*/ };
> >  
> >  #   define _STATBUF_ST_BLKSIZE
> >   




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andreas Schwab]

On Okt 14 2020, 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

Why can't you just change the type of st_ino in struct stat?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Hi Andreas,

> On Okt 14 2020, 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
> >  
> 
> Why can't you just change the type of st_ino in struct stat?

I'm not sure I get your idea. Could you be more specific?

> 
> Andreas.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andreas Schwab]

If you design a new layout of struct stat you are not constrained by
existing layouts.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Adhemerval Zanella]

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

It leads to define the struct __stat64_t64 on its own header instead 
of trying to accommodate it on stat.h header.  The idea it to eventually
export struct_stat_time64.h as an installed header and make the
required redirections on stat.h header for -D_TIME_SIZE=64 (so
stat64 redirects to __stat_time64).

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

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

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.

With current situation we do have
(at sysdeps/unix/sysv/linux/struct_stat_time64.h):

struct __stat64_t64
{
  __dev_t st_dev;
  __ino64_t st_ino;


Which is placed just after st_dev. 

In the struct stat{64} the st_ino is the last member.

As a result we do have a mismatch when passing binary data (the offsets
are different for st_ino).

Code from this patch moves st_ino to the last place, which saves a lot
of extra code - i.e. one for alleviating the casting of data.

Due to that also the patch in 
https://github.com/lmajewski/y2038_glibc/commit/26aa2ac07246682a505d85dac1c269689964b79b

can be small.


> 
> It leads to define the struct __stat64_t64 on its own header instead 
> of trying to accommodate it on stat.h header.  The idea it to
> eventually export struct_stat_time64.h as an installed header and
> make the required redirections on stat.h header for -D_TIME_SIZE=64
> (so stat64 redirects to __stat_time64).

As it is now - those two structures are not binary compatible due to
different st_ino placement (of course if we _only_ copy data with direct
assignment, then we are safe). 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Adhemerval Zanella]

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

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

Also for such cases __stat64_t64 *won't* be exported, its struct stat
*already* has 64-bit time fields.

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

In this case you will have:

  1. non-LFS {f,l}stat{64} that accepts 'struct stat'.
  2. LFS with 32-bit time fields {f,l}stat{64} that accepts 'struct stat64'.
  3. LFS with 64-bit time fields {f,l}fstat64_t64 (or any other
     name for the exported ABI) with accepts 'struct __stat64_t64'.

This is *different* than the current 32-bit y2038 enabled ABIs
(arc, riscv32) which does not need to support neither 1. nor 2..
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. 

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 (along with redefine the 'struct stat64' to
'struct __stat64_t64').

> 
> With current situation we do have
> (at sysdeps/unix/sysv/linux/struct_stat_time64.h):
> 
> struct __stat64_t64
> {
>   __dev_t st_dev;
>   __ino64_t st_ino;
> 
> 
> Which is placed just after st_dev. 
> 
> In the struct stat{64} the st_ino is the last member.
> 
> As a result we do have a mismatch when passing binary data (the offsets
> are different for st_ino).
> 
> Code from this patch moves st_ino to the last place, which saves a lot
> of extra code - i.e. one for alleviating the casting of data.
> 
> Due to that also the patch in 
> https://github.com/lmajewski/y2038_glibc/commit/26aa2ac07246682a505d85dac1c269689964b79b
> 
> can be small.
> 
> 
>>
>> It leads to define the struct __stat64_t64 on its own header instead 
>> of trying to accommodate it on stat.h header.  The idea it to
>> eventually export struct_stat_time64.h as an installed header and
>> make the required redirections on stat.h header for -D_TIME_SIZE=64
>> (so stat64 redirects to __stat_time64).
> 
> As it is now - those two structures are not binary compatible due to
> different st_ino placement (of course if we _only_ copy data with direct
> assignment, then we are safe). 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> 

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 6974 bytes --]

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

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



> 
> > 
> > With current situation we do have
> > (at sysdeps/unix/sysv/linux/struct_stat_time64.h):
> > 
> > struct __stat64_t64
> > {
> >   __dev_t st_dev;
> >   __ino64_t st_ino;
> > 
> > 
> > Which is placed just after st_dev. 
> > 
> > In the struct stat{64} the st_ino is the last member.
> > 
> > As a result we do have a mismatch when passing binary data (the
> > offsets are different for st_ino).
> > 
> > Code from this patch moves st_ino to the last place, which saves a
> > lot of extra code - i.e. one for alleviating the casting of data.
> > 
> > Due to that also the patch in 
> > https://github.com/lmajewski/y2038_glibc/commit/26aa2ac07246682a505d85dac1c269689964b79b
> > 
> > can be small.
> > 
> >   
> >>
> >> It leads to define the struct __stat64_t64 on its own header
> >> instead of trying to accommodate it on stat.h header.  The idea it
> >> to eventually export struct_stat_time64.h as an installed header
> >> and make the required redirections on stat.h header for
> >> -D_TIME_SIZE=64 (so stat64 redirects to __stat_time64).  
> > 
> > As it is now - those two structures are not binary compatible due to
> > different st_ino placement (of course if we _only_ copy data with
> > direct assignment, then we are safe). 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma@denx.de 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [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.

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 9852 bytes --]

Hi Adhemerval,

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

Ok. Now I do see that in e.g. struct_stat.h (@unix/sysv/linux/bits)
struct stat with -D_FILE_OFFSET_BITS=64 is similar to struct stat64

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

This is what I've just proposed (at least I do think so :-) ) to Joseph
in the other mail (you were CC'ed).

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

This code I do already have implemented [1]

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

I use code [1] for some time and it doesn't make any issues on
archs with WORDSIZE=64.


Links:
[1] - 
https://github.com/lmajewski/y2038_glibc/commit/c84de924e70be94e8c34437cf5f67bf914d77761


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Joseph Myers]

On Wed, 14 Oct 2020, Lukasz Majewski wrote:

> 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

Some basic principles for providing such support:

* struct stat and struct stat64 need updating simultaneously for all 
architectures that currently use 32-bit times.  There are 11 
bits/struct_stat.h headers under sysdeps/unix/sysv/linux/ (though some are 
for architectures with no 32-bit-time support); updating just one to 
handle _TIME_BITS=64 is not sufficient.

* Since __stat64_t64 has the same layout across architectures, it's 
strongly desirable to define its contents in only one place, rather than 
duplicating them in at least two copies in each of 11 headers.

This suggest some possibilities for complete but minimal changes:

* Define __STAT64_T64_CONTENT in some installed bits/ header and use it 
conditionally in each bits/struct_stat.h header.

#if __GLIBC_USE (TIME_BITS64)
  __STAT64_T64_CONTENT
#else
  /* existing struct contents */
#endif

* Make bits/stat.h contain the conditional:

#if __TIMESIZE == 32 && __GLIBC_USE (TIME_BITS64)
# include <bits/struct_stat_time64.h>
#else
# include <bits/struct_stat.h>
#endif

(where bits/struct_stat_time64.h is a new installed header that defines 
both struct stat and struct stat64 with the __stat64_t64 layout, maybe 
using a macro such as __STAT64_T64_CONTENT to avoid even duplication 
within the header).  I think this approach is more attractive because it 
avoids needing to make changes to lots of separate bits/struct_stat.h 
header.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Hi Joseph,

> On Wed, 14 Oct 2020, Lukasz Majewski wrote:
> 
> > 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
> >  
> 
> Some basic principles for providing such support:
> 
> * struct stat and struct stat64 need updating simultaneously for all 
> architectures that currently use 32-bit times.  There are 11 
> bits/struct_stat.h headers under sysdeps/unix/sysv/linux/ (though
> some are for architectures with no 32-bit-time support); updating
> just one to handle _TIME_BITS=64 is not sufficient.

Ok.

> 
> * Since __stat64_t64 has the same layout across architectures, it's 
> strongly desirable to define its contents in only one place, rather
> than duplicating them in at least two copies in each of 11 headers.
> 
> This suggest some possibilities for complete but minimal changes:
> 
> * Define __STAT64_T64_CONTENT in some installed bits/ header and use
> it conditionally in each bits/struct_stat.h header.

I've poked around glibc code and I cannot find such paradigm - i.e.
defining as C preprocessor macro members of a struct in exported headers
files and then pasting it to internal and external headers.

Is it safe in the respect of using such approach in e.g. ld-*
program building?

As fair as I see - all core exported structures modifications use
members modification with #ifdef __USE_FOO and then solid type is added.

> 
> #if __GLIBC_USE (TIME_BITS64)
>   __STAT64_T64_CONTENT
> #else
>   /* existing struct contents */
> #endif
> 
> * Make bits/stat.h contain the conditional:
> 
> #if __TIMESIZE == 32 && __GLIBC_USE (TIME_BITS64)
> # include <bits/struct_stat_time64.h>
> #else
> # include <bits/struct_stat.h>
> #endif
> 
> (where bits/struct_stat_time64.h is a new installed header that
> defines both struct stat and struct stat64 with the __stat64_t64
> layout, maybe using a macro such as __STAT64_T64_CONTENT to avoid
> even duplication within the header).  I think this approach is more
> attractive because it avoids needing to make changes to lots of
> separate bits/struct_stat.h header.
> 

Wouldn't it be better to just make struct __stat64_t64 exported in e.g.
sysdeps/unix/sysv/linux/bits/struct_stat_time64.h and then just make
an alias in exported struct_stat.h
(sysdeps/unix/sysv/linux/bits/struct_stat.h):

#ifdef __USE_TIME_BITS64
#include <bits/struct_stat_time64.h>
#define stat __stat64_t64
#else
struct stat {
...
}
#endif

(the same approach for exported struct stat64).

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Joseph Myers]

On Thu, 15 Oct 2020, Lukasz Majewski wrote:

> Wouldn't it be better to just make struct __stat64_t64 exported in e.g.
> sysdeps/unix/sysv/linux/bits/struct_stat_time64.h and then just make
> an alias in exported struct_stat.h
> (sysdeps/unix/sysv/linux/bits/struct_stat.h):
> 
> #ifdef __USE_TIME_BITS64
> #include <bits/struct_stat_time64.h>
> #define stat __stat64_t64

I don't think redirecting struct tags with #define like that is safe, at 
least not when the tag is also used as a function name as here.

-- 
Joseph S. Myers
joseph@codesourcery.com