[RFC,v6,06/23] sysdeps: Use long types with 64-bit time_t on 32-bit archs

  If we are using a 32-bit architecture with a 64-bit time_t let's
define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
expecting a type __kernel_old_time_t which is 32-bit even with a
64-bit time_t. Instead of adding another check, let's just set
__SEM_PAD_BEFORE_TIME as that will use a long type instead of
long long.
---
 sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
  

On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> If we are using a 32-bit architecture with a 64-bit time_t let's
> define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> expecting a type __kernel_old_time_t which is 32-bit even with a
> 64-bit time_t. Instead of adding another check, let's just set
> __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> long long.

I think you need to do this differently to avoid truncating the timestamp
to 32 bit. The type in the generic kernel headers for 32-bit targets is not
'__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is
'unsigned long sem_otime; unsigned long sem_otime_high;'.

Both need to be combined to read the 64-bit time_t value. It should
be possible to do this in an architecture independent way, but note
that the two halves are not always adjacent or in the correct order,
and in one case (mips shmbuf) the high word is only 16 bit instead
of 32.

          Arnd

> ---
>  sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> index 566ce039cc..4b419a6100 100644
> --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h
> +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> @@ -30,4 +30,15 @@
>     layout conversions for this structure.  */
>
>  #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32)
> -#define __SEM_PAD_BEFORE_TIME 0
> +
> +/* If we are using a 32-bit architecture with a 64-bit time_t let's
> +   define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> +   expecting a type __kernel_old_time_t which is 32-bit even with a
> +   64-bit time_t. Instead of adding another check, let's just set
> +   __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> +   long long.  */
> +#if __WORDSIZE == 32 && __TIMESIZE == 64
> +# define __SEM_PAD_BEFORE_TIME 1
> +#else
> +# define __SEM_PAD_BEFORE_TIME 0
> +#endif
> --
> 2.24.1
>
  

On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > If we are using a 32-bit architecture with a 64-bit time_t let's
> > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > expecting a type __kernel_old_time_t which is 32-bit even with a
> > 64-bit time_t. Instead of adding another check, let's just set
> > __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > long long.
>
> I think you need to do this differently to avoid truncating the timestamp
> to 32 bit. The type in the generic kernel headers for 32-bit targets is not
> '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is
> 'unsigned long sem_otime; unsigned long sem_otime_high;'.

Argh! That is even harder.

I think I have updated both sem_otime and sem_ctime to have a high and
low 32-bit version. That looks actually pretty straightforward.

It does seem like we will also have to stich the two values together
as bits/sem.h expects them to just be a time_t. That seems self
contained in sysdeps/unix/sysv/linux/semctl.c though.

>
> Both need to be combined to read the 64-bit time_t value. It should
> be possible to do this in an architecture independent way, but note
> that the two halves are not always adjacent or in the correct order,
> and in one case (mips shmbuf) the high word is only 16 bit instead
> of 32.

That complicates things. I can probably get away with adding a new
define, but that seems like a pain. The best bet looks like allowing
arches to set __SEM_PAD_TIME and then having each arch specify their
own. Maybe with some sane default and then have MIPS set it's own.

Do glibc develepers have a preference here?

Alistair

>
>           Arnd
>
> > ---
> >  sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > index 566ce039cc..4b419a6100 100644
> > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > @@ -30,4 +30,15 @@
> >     layout conversions for this structure.  */
> >
> >  #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32)
> > -#define __SEM_PAD_BEFORE_TIME 0
> > +
> > +/* If we are using a 32-bit architecture with a 64-bit time_t let's
> > +   define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > +   expecting a type __kernel_old_time_t which is 32-bit even with a
> > +   64-bit time_t. Instead of adding another check, let's just set
> > +   __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > +   long long.  */
> > +#if __WORDSIZE == 32 && __TIMESIZE == 64
> > +# define __SEM_PAD_BEFORE_TIME 1
> > +#else
> > +# define __SEM_PAD_BEFORE_TIME 0
> > +#endif
> > --
> > 2.24.1
> >
  

On Thu, Jan 16, 2020 at 5:30 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > If we are using a 32-bit architecture with a 64-bit time_t let's
> > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > > expecting a type __kernel_old_time_t which is 32-bit even with a
> > > 64-bit time_t. Instead of adding another check, let's just set
> > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > > long long.
> >
> > I think you need to do this differently to avoid truncating the timestamp
> > to 32 bit. The type in the generic kernel headers for 32-bit targets is not
> > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is
> > 'unsigned long sem_otime; unsigned long sem_otime_high;'.
>
> Argh! That is even harder.
>
> I think I have updated both sem_otime and sem_ctime to have a high and
> low 32-bit version. That looks actually pretty straightforward.
>
> It does seem like we will also have to stich the two values together
> as bits/sem.h expects them to just be a time_t. That seems self
> contained in sysdeps/unix/sysv/linux/semctl.c though.

Scratch that, sysdeps/unix/sysv/linux/bits/sem.h overwrites it so I
don't see how we could stitch the values together.

>
> >
> > Both need to be combined to read the 64-bit time_t value. It should
> > be possible to do this in an architecture independent way, but note
> > that the two halves are not always adjacent or in the correct order,
> > and in one case (mips shmbuf) the high word is only 16 bit instead
> > of 32.
>
> That complicates things. I can probably get away with adding a new
> define, but that seems like a pain. The best bet looks like allowing
> arches to set __SEM_PAD_TIME and then having each arch specify their
> own. Maybe with some sane default and then have MIPS set it's own.

This actually isn't bad, I just added this:

#define __SEM_NO_PAD (__WORDSIZE == 32 && __TIMESIZE == 64)

and forced it to 0 for MIPS.

Alistair

>
> Do glibc develepers have a preference here?
>
> Alistair
>
> >
> >           Arnd
> >
> > > ---
> > >  sysdeps/unix/sysv/linux/bits/sem-pad.h | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sysdeps/unix/sysv/linux/bits/sem-pad.h b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > > index 566ce039cc..4b419a6100 100644
> > > --- a/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > > +++ b/sysdeps/unix/sysv/linux/bits/sem-pad.h
> > > @@ -30,4 +30,15 @@
> > >     layout conversions for this structure.  */
> > >
> > >  #define __SEM_PAD_AFTER_TIME (__TIMESIZE == 32)
> > > -#define __SEM_PAD_BEFORE_TIME 0
> > > +
> > > +/* If we are using a 32-bit architecture with a 64-bit time_t let's
> > > +   define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > > +   expecting a type __kernel_old_time_t which is 32-bit even with a
> > > +   64-bit time_t. Instead of adding another check, let's just set
> > > +   __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > > +   long long.  */
> > > +#if __WORDSIZE == 32 && __TIMESIZE == 64
> > > +# define __SEM_PAD_BEFORE_TIME 1
> > > +#else
> > > +# define __SEM_PAD_BEFORE_TIME 0
> > > +#endif
> > > --
> > > 2.24.1
> > >
  

On Thu, Jan 16, 2020 at 8:31 AM Alistair Francis <alistair23@gmail.com> wrote:
> On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis <alistair.francis@wdc.com> wrote:
> > >
> > > If we are using a 32-bit architecture with a 64-bit time_t let's
> > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > > expecting a type __kernel_old_time_t which is 32-bit even with a
> > > 64-bit time_t. Instead of adding another check, let's just set
> > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > > long long.
> >
> > I think you need to do this differently to avoid truncating the timestamp
> > to 32 bit. The type in the generic kernel headers for 32-bit targets is not
> > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is
> > 'unsigned long sem_otime; unsigned long sem_otime_high;'.
>
> Argh! That is even harder.
>
> I think I have updated both sem_otime and sem_ctime to have a high and
> low 32-bit version. That looks actually pretty straightforward.
>
> It does seem like we will also have to stich the two values together
> as bits/sem.h expects them to just be a time_t. That seems self
> contained in sysdeps/unix/sysv/linux/semctl.c though.
>
> >
> > Both need to be combined to read the 64-bit time_t value. It should
> > be possible to do this in an architecture independent way, but note
> > that the two halves are not always adjacent or in the correct order,
> > and in one case (mips shmbuf) the high word is only 16 bit instead
> > of 32.
>
> That complicates things. I can probably get away with adding a new
> define, but that seems like a pain. The best bet looks like allowing
> arches to set __SEM_PAD_TIME and then having each arch specify their
> own. Maybe with some sane default and then have MIPS set it's own.

There are three approaches that we have discussed in the past:

1. Convert the timestamps in place where possible, with special cases
    for architectures that have the upper halves in non-consecutive
    locations
2. Create a new set of portable sysvipc data structures for time64
    user space, e.g. modeled after the generic 64-bit structures,
    and convert all fields between the kernel version and the libc
    version.
3. Keep the current layout but rename the existing time fields and
    add new 64-bit fields at the end.

You can have a look at the musl implementation for the third approach,
the main advantage there is that it simplifies conversion between
the old and new format for supporting the time32 interfaces on
the existing 32-bit architectures as wrappers on top of the tim64
interfaces.

There is also a simplified form of 1) that will work on most of the
architectures that glibc cares about: i386, armel, rv32le, csky, nds32,
ppc32, microblaze-le, sparc, parisc, sh-le and nios2 all have the 'high'
fields in the right place so you can simply treat them as 64-bit
fields with no padding, while armeb, mips (all of them), s390,
microblaze-be, sh-be and m68k get it wrong in one way or another
and require would require some special case here.

           Arnd
  

On Thu, Jan 16, 2020 at 6:36 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jan 16, 2020 at 8:31 AM Alistair Francis <alistair23@gmail.com> wrote:
> > On Mon, Jan 13, 2020 at 7:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jan 12, 2020 at 11:40 AM Alistair Francis <alistair.francis@wdc.com> wrote:
> > > >
> > > > If we are using a 32-bit architecture with a 64-bit time_t let's
> > > > define __SEM_PAD_BEFORE_TIME as 1. This is because the kernel is
> > > > expecting a type __kernel_old_time_t which is 32-bit even with a
> > > > 64-bit time_t. Instead of adding another check, let's just set
> > > > __SEM_PAD_BEFORE_TIME as that will use a long type instead of
> > > > long long.
> > >
> > > I think you need to do this differently to avoid truncating the timestamp
> > > to 32 bit. The type in the generic kernel headers for 32-bit targets is not
> > > '__kernel_old_time_t sem_otime; __kernel_long_t pad' but it is
> > > 'unsigned long sem_otime; unsigned long sem_otime_high;'.
> >
> > Argh! That is even harder.
> >
> > I think I have updated both sem_otime and sem_ctime to have a high and
> > low 32-bit version. That looks actually pretty straightforward.
> >
> > It does seem like we will also have to stich the two values together
> > as bits/sem.h expects them to just be a time_t. That seems self
> > contained in sysdeps/unix/sysv/linux/semctl.c though.
> >
> > >
> > > Both need to be combined to read the 64-bit time_t value. It should
> > > be possible to do this in an architecture independent way, but note
> > > that the two halves are not always adjacent or in the correct order,
> > > and in one case (mips shmbuf) the high word is only 16 bit instead
> > > of 32.
> >
> > That complicates things. I can probably get away with adding a new
> > define, but that seems like a pain. The best bet looks like allowing
> > arches to set __SEM_PAD_TIME and then having each arch specify their
> > own. Maybe with some sane default and then have MIPS set it's own.
>
> There are three approaches that we have discussed in the past:
>
> 1. Convert the timestamps in place where possible, with special cases
>     for architectures that have the upper halves in non-consecutive
>     locations
> 2. Create a new set of portable sysvipc data structures for time64
>     user space, e.g. modeled after the generic 64-bit structures,
>     and convert all fields between the kernel version and the libc
>     version.
> 3. Keep the current layout but rename the existing time fields and
>     add new 64-bit fields at the end.

Ah ok, I prefer option 1 I think.

So I guess we will do something like:

 1. Define __semid_ds32 in sysdeps/unix/sysv/linux/bits/sem.h based on
macros in sem-pad.h
    - This allows us to have a generic implementation in
sysdeps/unix/sysv/linux/bits that can be overwritten by other
architectures
 2. If 32-bit with 64-bit time_t pass __semid_ds32 into the kernel in
sysdeps/unix/sysv/linux/semctl.c
    - Then reconstruct the user exposed version (unchanged). This is
the current struct semid_ds and uses time_t
    - The problem is not every architecture uses time_t here, that
only seems to apply if __TIMESIZE == 32 so we should be ok here.

Alistair

>
> You can have a look at the musl implementation for the third approach,
> the main advantage there is that it simplifies conversion between
> the old and new format for supporting the time32 interfaces on
> the existing 32-bit architectures as wrappers on top of the tim64
> interfaces.
>
> There is also a simplified form of 1) that will work on most of the
> architectures that glibc cares about: i386, armel, rv32le, csky, nds32,
> ppc32, microblaze-le, sparc, parisc, sh-le and nios2 all have the 'high'
> fields in the right place so you can simply treat them as 64-bit
> fields with no padding, while armeb, mips (all of them), s390,
> microblaze-be, sh-be and m68k get it wrong in one way or another
> and require would require some special case here.
>
>            Arnd
  

On Fri, 17 Jan 2020, Alistair Francis wrote:

>  1. Define __semid_ds32 in sysdeps/unix/sysv/linux/bits/sem.h based on
> macros in sem-pad.h

The usual rule applies that internal structures generally do not belong in 
installed headers.

sysdeps/unix/sysv/linux/bits/sem.h should define the *public* struct 
semid_ds.  That public definition may eventually vary based on some #if 
conditions (__USE_TIME_BITS64 or whatever _TIME_BITS=64 causes to be 
defined - not yet, only once we're actually ready to support _TIME_BITS=64 
as a complete, consistent interface).  But if the purpose of a definition 
is to pass into the kernel, rather than as a public API (under a public 
name) for user programs, it does not belong in an installed header - 
unless it's e.g. referenced in some other public structure in some way.