[RFC v4 00/24] RISC-V glibc port for the 32-bit

classic Classic list List threaded Threaded
73 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Joseph Myers
On Mon, 12 Aug 2019, Zack Weinberg wrote:

>  - Our implementation of gettimeofday should always pass NULL for
> struct timezone to the kernel, and write zeroes to any struct timezone
> argument that is supplied.  (This will transitively apply to ftime.)

Given that the kernel timezone is in fact meaningful (for the kernel's
interpretation of data shared with other OSes, such as the RTC clock and
some filesystem timestamps, that is kept in local time - just not for the
purpose for which the timezone settings in gettimeofday / settimeofday
were originally intended), I think gettimeofday should continue to read
that information from the kernel when the kernel provides it (i.e., the
uses of the kernel timezone mean that other syscalls do *not* supersede
the gettimeofday syscall for this purpose and so access to that
information from that syscall should continue to be provided).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Zack Weinberg-2
On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <[hidden email]> wrote:

>
> On Mon, 12 Aug 2019, Zack Weinberg wrote:
>
> >  - Our implementation of gettimeofday should always pass NULL for
> > struct timezone to the kernel, and write zeroes to any struct timezone
> > argument that is supplied.  (This will transitively apply to ftime.)
>
> Given that the kernel timezone is in fact meaningful (for the kernel's
> interpretation of data shared with other OSes, such as the RTC clock and
> some filesystem timestamps, that is kept in local time - just not for the
> purpose for which the timezone settings in gettimeofday / settimeofday
> were originally intended), I think gettimeofday should continue to read
> that information from the kernel when the kernel provides it

Insisting on this would mean that we'd have to go back to the kernel
people to request a new API _before_ we could proceed with the time64
transition.  I don't like that idea.  Also, unlike settimeofday where
we know there is at least one real, intentional user, I am betting
that all or nearly all existing uses of gettimeofday with a non-NULL
timezone argument are actually bugs.

Instead, what if we add Linux-specific set_hw_timezone(const struct
timezone *) and get_hw_timezone(struct timezone *) (sys/timex.h seems
like an appropriate place for the prototypes) that will call
settimeofday/gettimeofday now, or whatever new interface the kernel
people invent to replace this function, in the future.  We can then
proceed to make both gettimeofday and settimeofday behave-as-if they
always calls clock_[gs]ettime(CLOCK_REALTIME) under the hood, and this
issue is decoupled from the time64 transition.

zw
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Joseph Myers
On Mon, 12 Aug 2019, Zack Weinberg wrote:

> On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <[hidden email]> wrote:
> >
> > On Mon, 12 Aug 2019, Zack Weinberg wrote:
> >
> > >  - Our implementation of gettimeofday should always pass NULL for
> > > struct timezone to the kernel, and write zeroes to any struct timezone
> > > argument that is supplied.  (This will transitively apply to ftime.)
> >
> > Given that the kernel timezone is in fact meaningful (for the kernel's
> > interpretation of data shared with other OSes, such as the RTC clock and
> > some filesystem timestamps, that is kept in local time - just not for the
> > purpose for which the timezone settings in gettimeofday / settimeofday
> > were originally intended), I think gettimeofday should continue to read
> > that information from the kernel when the kernel provides it
>
> Insisting on this would mean that we'd have to go back to the kernel
> people to request a new API _before_ we could proceed with the time64

No, but it might mean that these two functions are exceptions to the
general rule of functions for 32-bit time being thin wrappers round their
64-bit counterparts.  (For the existing ABIs I think keeping interfacing
with the kernel timezone is also a matter of ABI compatibility for these
functions, even if the new _TIME_BITS=64 version of gettimeofday just
writes zeroes to the timezone.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Alistair Francis-2
In reply to this post by Zack Weinberg-2
On Mon, Aug 12, 2019 at 8:42 AM Zack Weinberg <[hidden email]> wrote:

>
> On Fri, Aug 9, 2019 at 9:04 PM Alistair Francis
> <[hidden email]> wrote:
> >
> > Append the struct timezone members with '_dep'. This indicates that
> > these members are deprecated and will cause build failures on code that
> > is currently using the members.
>
> I had been working on a more thorough version of this patch, one that
> removed these fields altogether (replacing them with `char
> __preserve_historic_size[2*sizeof(int)]`) and changed _all_ of the
> code that touches them, within glibc, to either ignore the fields or
> zero them out.  This patch tripped over an Alpha-only ABI headache
> (Alpha has compatibility versions of both gettimeofday and
> settimeofday) and then I ran out of time to work on it.
>
> In view of the reported behavior of systemd, though, I now think
> neither your approach nor mine is exactly what we should do.  We
> should do something along these lines for the next release:
>
>  - We should _not_ rename the fields of struct timezone, but we should
> mark them __attribute_deprecated__ so that code still using them
> triggers compile warnings.

This sounds good to me

>  - We should do the same for the timezone fields of struct timeb.
>  - Our implementation of gettimeofday should always pass NULL for
> struct timezone to the kernel, and write zeroes to any struct timezone
> argument that is supplied.  (This will transitively apply to ftime.)

I don't think this is the right idea here and reading further down the
list it seems like not everyone is on board.

>  - We should leave settimeofday alone until the kernel has an
> alternative means of doing the "RTC clock warp" thing that systemd is
> trying to do.

Agreed, at least for systems that have a settimeofday syscall.

>
> Do you have time to work that patch up?  You can get my
> patch-in-progress from
> https://sourceware.org/git/?p=glibc.git;a=log;h=refs/heads/zack/gtod-no-timezone
> (please grab at least the changes to manual/time.texi).  If you don't
> have time, I will try to find time to work on it this week.

I do have time and am happy to do it, the problem is that WDC still
doesn't have a FSF contributor agreement so from my understanding the
patch couldn't be applied.

Alistair

>
> zw
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Alistair Francis-2
In reply to this post by Joseph Myers
On Mon, Aug 12, 2019 at 2:26 PM Joseph Myers <[hidden email]> wrote:

>
> On Mon, 12 Aug 2019, Zack Weinberg wrote:
>
> > On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <[hidden email]> wrote:
> > >
> > > On Mon, 12 Aug 2019, Zack Weinberg wrote:
> > >
> > > >  - Our implementation of gettimeofday should always pass NULL for
> > > > struct timezone to the kernel, and write zeroes to any struct timezone
> > > > argument that is supplied.  (This will transitively apply to ftime.)
> > >
> > > Given that the kernel timezone is in fact meaningful (for the kernel's
> > > interpretation of data shared with other OSes, such as the RTC clock and
> > > some filesystem timestamps, that is kept in local time - just not for the
> > > purpose for which the timezone settings in gettimeofday / settimeofday
> > > were originally intended), I think gettimeofday should continue to read
> > > that information from the kernel when the kernel provides it
> >
> > Insisting on this would mean that we'd have to go back to the kernel
> > people to request a new API _before_ we could proceed with the time64
>
> No, but it might mean that these two functions are exceptions to the
> general rule of functions for 32-bit time being thin wrappers round their
> 64-bit counterparts.  (For the existing ABIs I think keeping interfacing
> with the kernel timezone is also a matter of ABI compatibility for these
> functions, even if the new _TIME_BITS=64 version of gettimeofday just
> writes zeroes to the timezone.)

This is what I was picturing as well, with the individual members
marked as deprecated.

Alistair

>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Alistair Francis-2
In reply to this post by Joseph Myers
On Mon, Aug 12, 2019 at 1:01 PM Joseph Myers <[hidden email]> wrote:

>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> > When copying the statx struct to the stat stuct use the original stat
> > struct instead of the stat64 struct. As the padding in the original is
> > type 'unsigned short int' but the padding in the stat64 is 'unsigned int'
> > the copy  can result in misallgined data. This would then incorrectly
> > trigger the stat_overflow() failure.
>
> This indicates there's something else wrong with the port.  By design, the
> following apply for linux/generic/wordsize-32 ports of glibc:
>
> * The layout of struct stat and struct stat64 is identical, except that
> some bytes that are padding in struct stat serve as high parts of fields
> that are wider in struct stat64 (and thus have endian-dependent positions
> as determined by the __field64 macro in bits/stat.h).

__ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
same thing. The __pad1 changes in size between the two structs as one
is a short and one is just an int. I don't see how they are identical.

>
> * Conversions from statx have to go to stat64, including setting those
> high parts as appropriate, so that the subsequent overflow checks (which
> work by examining those padding fields) can correctly detect whether
> overflow occurred and set errno to EOVERFLOW accordingly.  See my C-SKY
> port reviews that resulted in the code we have now
> <https://sourceware.org/ml/libc-alpha/2018-11/msg00624.html>
> <https://sourceware.org/ml/libc-alpha/2018-11/msg00668.html>.

Ok, this makes sense.

>
> > This would be very obvious when using a 64-bit ino_t type on a 32-bit
> > system, such as the RV32 port.
>
> If those types are 64-bit, you should not have padding around them in
> struct stat, so as to preserve the property that struct stat and struct
> stat64 have the same layout.  I suppose this means bits/stat.h needs to
> check further macros such as __OFF_T_MATCHES_OFF64_T.

Changing the padding would fix the problem. Just to be clear is that
what you are suggesting?

>
> You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1.  And
> you'll need to make wordsize-32/overflow.h define trivial versions of the
> *_overflow functions in cases where the types match (this should be done
> in that file, rather than making an RV32-specific copy, to benefit future
> ports that make the same choices as RV32).

I think I tested that and it didn't fix the problem so I dropped it.
I'll add the XSTAT_IS_XSTAT64 define back.

I'm not sure what you mean by then the types match?

Alistair

>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Joseph Myers
On Mon, 12 Aug 2019, Alistair Francis wrote:

> > * The layout of struct stat and struct stat64 is identical, except that
> > some bytes that are padding in struct stat serve as high parts of fields
> > that are wider in struct stat64 (and thus have endian-dependent positions
> > as determined by the __field64 macro in bits/stat.h).
>
> __ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
> same thing. The __pad1 changes in size between the two structs as one
> is a short and one is just an int. I don't see how they are identical.

What file are you looking at, in what version of the source tree?

I'm looking at sysdeps/unix/sysv/linux/generic/bits/stat.h.  As far as I
can tell, your patch series doesn't change that file.  That's the version
used by RV64 and it should be used by RV32 (the headers installed for RV32
and RV64 should be identical apart from the special cases of
gnu/lib-names-*.h and gnu/stubs-*.h; if it isn't, that needs to be fixed).

In that file, __pad1 is of type __dev_t in both struct stat and struct
stat64.

> > If those types are 64-bit, you should not have padding around them in
> > struct stat, so as to preserve the property that struct stat and struct
> > stat64 have the same layout.  I suppose this means bits/stat.h needs to
> > check further macros such as __OFF_T_MATCHES_OFF64_T.
>
> Changing the padding would fix the problem. Just to be clear is that
> what you are suggesting?

Yes.

Either change the definition of __field64, or the uses of it.

Changing the definition runs into the complication that it's used three
times and each has its own macro to say whether the two types in question
match, but you could always have a #error if some match and others don't.

> > You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1.  And
> > you'll need to make wordsize-32/overflow.h define trivial versions of the
> > *_overflow functions in cases where the types match (this should be done
> > in that file, rather than making an RV32-specific copy, to benefit future
> > ports that make the same choices as RV32).
>
> I think I tested that and it didn't fix the problem so I dropped it.
> I'll add the XSTAT_IS_XSTAT64 define back.
>
> I'm not sure what you mean by then the types match?

off_t and off64_t match if they have the same size and alignment (if
__OFF_T_MATCHES_OFF64_T is defined).  Likewise for the other pairs.

If off_t and off64_t match, there is no need for the "buf->__st_size_pad
== 0" check in stat_overflow (and once you've removed the padding in that
case, that check won't even compile because __st_size_pad won't exist).  
Likewise for the other pairs.  In practice, you can probably just make
that function return 0 if all three macros are defined, rather than
allowing for arbitrary subsets of the types matching or not matching.

Similar considerations apply to statfs_overflow if the types and padding
used in struct statfs match those used in struct statfs64.  So apply
similar considerations to the two versions of that structure.

You probably don't need to do anything special with lseek_overflow because
the compiler should just optimize that away when off_t and loff_t are the
same type.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Alistair Francis-2
On Mon, Aug 12, 2019 at 4:02 PM Joseph Myers <[hidden email]> wrote:

>
> On Mon, 12 Aug 2019, Alistair Francis wrote:
>
> > > * The layout of struct stat and struct stat64 is identical, except that
> > > some bytes that are padding in struct stat serve as high parts of fields
> > > that are wider in struct stat64 (and thus have endian-dependent positions
> > > as determined by the __field64 macro in bits/stat.h).
> >
> > __ino_t is a 64-bit value in RV32, so in both stat and stat64 it's the
> > same thing. The __pad1 changes in size between the two structs as one
> > is a short and one is just an int. I don't see how they are identical.
>
> What file are you looking at, in what version of the source tree?

I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
definition then the one you are looking at.

>
> I'm looking at sysdeps/unix/sysv/linux/generic/bits/stat.h.  As far as I
> can tell, your patch series doesn't change that file.  That's the version
> used by RV64 and it should be used by RV32 (the headers installed for RV32
> and RV64 should be identical apart from the special cases of
> gnu/lib-names-*.h and gnu/stubs-*.h; if it isn't, that needs to be fixed).

It looks like __field64(__ino_t, __ino64_t, st_ino) will use type int
for RV32 (I don't think __USE_FILE_OFFSET64 is defined) which might
need some changes.

>
> In that file, __pad1 is of type __dev_t in both struct stat and struct
> stat64.

In the file you are looking at that is correct.

>
> > > If those types are 64-bit, you should not have padding around them in
> > > struct stat, so as to preserve the property that struct stat and struct
> > > stat64 have the same layout.  I suppose this means bits/stat.h needs to
> > > check further macros such as __OFF_T_MATCHES_OFF64_T.
> >
> > Changing the padding would fix the problem. Just to be clear is that
> > what you are suggesting?
>
> Yes.
>
> Either change the definition of __field64, or the uses of it.

Yep, in the file you are looking at that seems the best way to go.

>
> Changing the definition runs into the complication that it's used three
> times and each has its own macro to say whether the two types in question
> match, but you could always have a #error if some match and others don't.
>
> > > You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1.  And
> > > you'll need to make wordsize-32/overflow.h define trivial versions of the
> > > *_overflow functions in cases where the types match (this should be done
> > > in that file, rather than making an RV32-specific copy, to benefit future
> > > ports that make the same choices as RV32).
> >
> > I think I tested that and it didn't fix the problem so I dropped it.
> > I'll add the XSTAT_IS_XSTAT64 define back.
> >
> > I'm not sure what you mean by then the types match?
>
> off_t and off64_t match if they have the same size and alignment (if
> __OFF_T_MATCHES_OFF64_T is defined).  Likewise for the other pairs.
>
> If off_t and off64_t match, there is no need for the "buf->__st_size_pad
> == 0" check in stat_overflow (and once you've removed the padding in that
> case, that check won't even compile because __st_size_pad won't exist).
> Likewise for the other pairs.  In practice, you can probably just make
> that function return 0 if all three macros are defined, rather than
> allowing for arbitrary subsets of the types matching or not matching.

Ok, I think I understand. I'll update the patch series.

>
> Similar considerations apply to statfs_overflow if the types and padding
> used in struct statfs match those used in struct statfs64.  So apply
> similar considerations to the two versions of that structure.

Ok

Alistair

>
> You probably don't need to do anything special with lseek_overflow because
> the compiler should just optimize that away when off_t and loff_t are the
> same type.
>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Zack Weinberg-2
In reply to this post by Joseph Myers
On Mon, Aug 12, 2019 at 5:26 PM Joseph Myers <[hidden email]> wrote:

> On Mon, 12 Aug 2019, Zack Weinberg wrote:
> > On Mon, Aug 12, 2019 at 4:20 PM Joseph Myers <[hidden email]> wrote:
> > > On Mon, 12 Aug 2019, Zack Weinberg wrote:
> > >
> > > >  - Our implementation of gettimeofday should always pass NULL for
> > > > struct timezone to the kernel, and write zeroes to any struct timezone
> > > > argument that is supplied.  (This will transitively apply to ftime.)
> > >
> > > Given that the kernel timezone is in fact meaningful (for the kernel's
> > > interpretation of data shared with other OSes, such as the RTC clock and
> > > some filesystem timestamps, that is kept in local time - just not for the
> > > purpose for which the timezone settings in gettimeofday / settimeofday
> > > were originally intended), I think gettimeofday should continue to read
> > > that information from the kernel when the kernel provides it
> >
> > Insisting on this would mean that we'd have to go back to the kernel
> > people to request a new API _before_ we could proceed with the time64
>
>  (For the existing ABIs I think keeping interfacing
> with the kernel timezone is also a matter of ABI compatibility for these
> functions, even if the new _TIME_BITS=64 version of gettimeofday just
> writes zeroes to the timezone.)

I really can't agree with that.  At all.

The kernel time zone is a trip hazard for the overwhelming majority of
applications.  We would probably be _fixing bugs_ (well, rendering
them harmless) with the above proposed change to gettimeofday.

Also, I think having gettimeofday behave differently between different
architectures is a bad idea, and having gettimeofday behave
differently depending on _TIME_BITS is an even worse idea.  And we
know we _can't_ have gettimeofday mimic the old behavior when the only
available get-time syscall is clock_gettime, so...

zw
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Joseph Myers
In reply to this post by Alistair Francis-2
On Mon, 12 Aug 2019, Alistair Francis wrote:

> I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
> definition then the one you are looking at.

That file is not relevant for any Linux kernel port added in the past
several years.  All recent ports should use the asm-generic structure
layouts.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Paul Eggert
In reply to this post by Joseph Myers
Joseph Myers wrote:
> it might mean that these two functions are exceptions to the
> general rule of functions for 32-bit time being thin wrappers round their
> 64-bit counterparts.

If I'm understanding you correctly, you're suggesting that the existing-ABI
get/settimeofday calls behave as before (albeit with __attribute_deprecated__
warnings at compile-time), whereas the new-ABI gettimeofday clears *TZ if TZ is
nonnull and the new-ABI settimeofday ignores TZ.  That way, the one or two
ancient apps that rely on this stuff will still work if compiled in 32-bit
time_t mode. This sounds like a good way to go forward.

It might also make sense for the new-ABI gettimeofday to simply ignore its TZ
argument, as that would be easier to document and explain (plus a bit faster for
the typical gettimeofday case).
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Zack Weinberg-2
On Mon, Aug 12, 2019 at 8:53 PM Paul Eggert <[hidden email]> wrote:
> Joseph Myers wrote:
> > it might mean that these two functions are exceptions to the
> > general rule of functions for 32-bit time being thin wrappers round their
> > 64-bit counterparts.
>
> If I'm understanding you correctly, you're suggesting that the existing-ABI
> get/settimeofday calls behave as before (albeit with __attribute_deprecated__
> warnings at compile-time), whereas the new-ABI gettimeofday clears *TZ if TZ is
> nonnull and the new-ABI settimeofday ignores TZ.

This is also what I understand Joseph to be suggesting.

> That way, the one or two
> ancient apps that rely on this stuff will still work if compiled in 32-bit
> time_t mode. This sounds like a good way to go forward.

However, I don't agree with this assessment of it I think it would be
confusing, and potentially a source of bugs that only affect less
popular architectures, if new-ABI and old-ABI gettimeofday don't treat
the TZ argument the same.  And I suspect that any "ancient apps" are
actually mishandling time zones and we'd be doing them a favor by
making them think they're operating in UTC from now on.

(systemd's use of the timezone argument to settimeofday is a separate issue.)

> It might also make sense for the new-ABI gettimeofday to simply ignore its TZ
> argument, as that would be easier to document and explain (plus a bit faster for
> the typical gettimeofday case).

That's not safe; callers that pass a non-NULL TZ argument to
gettimeofday may well rely on it writing _something_ there.  If it
doesn't, they'll be operating on stack garbage.
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Alistair Francis-2
In reply to this post by Joseph Myers
On Mon, Aug 12, 2019 at 4:31 PM Joseph Myers <[hidden email]> wrote:
>
> On Mon, 12 Aug 2019, Alistair Francis wrote:
>
> > I'm looking at sysdeps/unix/sysv/linux/bits/stat.h, it has a different
> > definition then the one you are looking at.
>
> That file is not relevant for any Linux kernel port added in the past
> several years.  All recent ports should use the asm-generic structure
> layouts.

Ok, now I understand. I have made all of the changes you requested.

Alistair

>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 05/24] sysdeps/clock_gettime: Use clock_gettime64 if avaliable

Alistair Francis-2
In reply to this post by Joseph Myers
On Mon, Aug 12, 2019 at 12:46 PM Joseph Myers <[hidden email]> wrote:

>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> >  /* Get current value of CLOCK and store it in TP.  */
> > +
> >  int
> >  __clock_gettime (clockid_t clock_id, struct timespec *tp)
> >  {
> > -  return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> > +
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +   return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> > +#else
>
> This has the usual problems with missing conversions.

I was under the impression (although possibly incorrectly) that if
__ASSUME_TIME64_SYSCALLS is defined we would have a 64-bit time_t. Is
__ASSUME_TIME64_SYSCALLS going to be defined on 32-bit architectures
that are still using a 32-bit time_t?

Alistair

>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 05/24] sysdeps/clock_gettime: Use clock_gettime64 if avaliable

Joseph Myers
On Tue, 13 Aug 2019, Alistair Francis wrote:

> I was under the impression (although possibly incorrectly) that if
> __ASSUME_TIME64_SYSCALLS is defined we would have a 64-bit time_t. Is
> __ASSUME_TIME64_SYSCALLS going to be defined on 32-bit architectures
> that are still using a 32-bit time_t?

Yes.  --enable-kernel=5.1.0 is not an ABI-changing option; it only affects
what syscalls glibc can assume to be present at runtime in the kernel it
runs on.

__ASSUME_TIME64_SYSCALLS means only that certain syscalls using 64-bit
time are assumed to be available: either suffixed versions such as
__NR_clock_gettime64, or corresponding unsuffixed versions such as
__NR_clock_gettime in the case where those use 64-bit time and have
exactly the same semantics and so "#define __NR_clock_gettime64
__NR_clock_gettime" is OK.  (Because the kernel headers must be at least
as recent as the minimum kernel version used at runtime, it also implies
that the kernel headers do define the syscall numbers - but as noted, the
syscalls might be __NR_clock_gettime etc. rather than the suffixed
versions.)

All four combinations of __ASSUME_TIME64_SYSCALLS and __TIMESIZE are
valid.

1. __TIMESIZE == 64 and __ASSUME_TIME64_SYSCALLS defined means a glibc
port does not support anything other than 64-bit time *as its userspace
ABI*, with the minimum kernel version ensuring appropriate syscalls are
available.  Cases of this include:

(a) Classic 64-bit systems (__WORDSIZE == 64, unsuffixed syscalls).

(b) x32 (__WORDSIZE == 32, __SYSCALL_WORDSIZE == 64, unsuffixed syscalls).

(c) New glibc ports with 32-bit "long int" in the syscall interface
(__WORDSIZE == 32 and __SYSCALL_WORDSIZE == 32) but only supporting 64-bit
time as its userspace ABI and with the configured minimum kernel version
(5.1 or later) ensuring appropriate (suffixed) syscalls are available (and
unsuffixed ones don't exist at all).  This is the case that applies to
RV32.

2. __TIMESIZE == 64 and __ASSUME_TIME64_SYSCALLS not defined means a glibc
port that does not support anything other than 64-bit time *as its
userspace ABI*, but with the minimum kernel version not ensuring
appropriate syscalls are available.  This implies __WORDSIZE == 32 and
__SYSCALL_WORDSIZE == 32.  This is a hypothetical case, if a new glibc
port were added for a 32-bit architecture that was supported in the Linux
kernel before 5.1, and if it were chosen to have 64-bit time as the
userspace ABI for this port but to support using kernels before 5.1.  As
it's hypothetical, it's not something you're expected to test unless
adding such a port - but most of the support for it should naturally fall
out from the Y2038 implementation anyway (the support for 64-bit
interfaces to use 32-bit syscalls when run on older kernels).

3. __TIMESIZE == 32 and __ASSUME_TIME64_SYSCALLS defined means a 32-bit
glibc port, where the unsuffixed time-related ABIs in glibc use 32-bit
time but the Y2038 work will add corresponding suffixed ABIs (and
_TIME_BITS headers support) that use 64-bit time - and where the minimum
kernel version ensures 64-bit-time (suffixed) syscalls are available.  So
the suffixed ABIs for 64-bit time can call those suffixed syscalls
unconditionally, but the unsuffixed ABIs need to do appropriate
translation between 32-bit and 64-bit time to call those syscalls (and the
expected design is that the unsuffixed ABIs will generally be thin
wrappers that do such translation and call the suffixed ones).

4. __TIMESIZE == 32 and __ASSUME_TIME64_SYSCALLS not defined means a
32-bit glibc port, where the unsuffixed time-related ABIs in glibc use
32-bit time but the Y2038 work will add corresponding suffixed ABIs (and
_TIME_BITS headers support) that use 64-bit time - and where the minimum
kernel version does not ensure that 64-bit-time (suffixed) syscalls are
available.  Thus, the suffixed ABIs for 64-bit time need to have support
for falling back to the 32-bit syscalls when the 64-bit ones are not
available.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 05/24] sysdeps/clock_gettime: Use clock_gettime64 if avaliable

Alistair Francis-2
On Wed, Aug 14, 2019 at 9:37 AM Joseph Myers <[hidden email]> wrote:

>
> On Tue, 13 Aug 2019, Alistair Francis wrote:
>
> > I was under the impression (although possibly incorrectly) that if
> > __ASSUME_TIME64_SYSCALLS is defined we would have a 64-bit time_t. Is
> > __ASSUME_TIME64_SYSCALLS going to be defined on 32-bit architectures
> > that are still using a 32-bit time_t?
>
> Yes.  --enable-kernel=5.1.0 is not an ABI-changing option; it only affects
> what syscalls glibc can assume to be present at runtime in the kernel it
> runs on.
>
> __ASSUME_TIME64_SYSCALLS means only that certain syscalls using 64-bit
> time are assumed to be available: either suffixed versions such as
> __NR_clock_gettime64, or corresponding unsuffixed versions such as
> __NR_clock_gettime in the case where those use 64-bit time and have
> exactly the same semantics and so "#define __NR_clock_gettime64
> __NR_clock_gettime" is OK.  (Because the kernel headers must be at least
> as recent as the minimum kernel version used at runtime, it also implies
> that the kernel headers do define the syscall numbers - but as noted, the
> syscalls might be __NR_clock_gettime etc. rather than the suffixed
> versions.)
>
> All four combinations of __ASSUME_TIME64_SYSCALLS and __TIMESIZE are
> valid.
>
> 1. __TIMESIZE == 64 and __ASSUME_TIME64_SYSCALLS defined means a glibc
> port does not support anything other than 64-bit time *as its userspace
> ABI*, with the minimum kernel version ensuring appropriate syscalls are
> available.  Cases of this include:
>
> (a) Classic 64-bit systems (__WORDSIZE == 64, unsuffixed syscalls).
>
> (b) x32 (__WORDSIZE == 32, __SYSCALL_WORDSIZE == 64, unsuffixed syscalls).
>
> (c) New glibc ports with 32-bit "long int" in the syscall interface
> (__WORDSIZE == 32 and __SYSCALL_WORDSIZE == 32) but only supporting 64-bit
> time as its userspace ABI and with the configured minimum kernel version
> (5.1 or later) ensuring appropriate (suffixed) syscalls are available (and
> unsuffixed ones don't exist at all).  This is the case that applies to
> RV32.
>
> 2. __TIMESIZE == 64 and __ASSUME_TIME64_SYSCALLS not defined means a glibc
> port that does not support anything other than 64-bit time *as its
> userspace ABI*, but with the minimum kernel version not ensuring
> appropriate syscalls are available.  This implies __WORDSIZE == 32 and
> __SYSCALL_WORDSIZE == 32.  This is a hypothetical case, if a new glibc
> port were added for a 32-bit architecture that was supported in the Linux
> kernel before 5.1, and if it were chosen to have 64-bit time as the
> userspace ABI for this port but to support using kernels before 5.1.  As
> it's hypothetical, it's not something you're expected to test unless
> adding such a port - but most of the support for it should naturally fall
> out from the Y2038 implementation anyway (the support for 64-bit
> interfaces to use 32-bit syscalls when run on older kernels).
>
> 3. __TIMESIZE == 32 and __ASSUME_TIME64_SYSCALLS defined means a 32-bit
> glibc port, where the unsuffixed time-related ABIs in glibc use 32-bit
> time but the Y2038 work will add corresponding suffixed ABIs (and
> _TIME_BITS headers support) that use 64-bit time - and where the minimum
> kernel version ensures 64-bit-time (suffixed) syscalls are available.  So
> the suffixed ABIs for 64-bit time can call those suffixed syscalls
> unconditionally, but the unsuffixed ABIs need to do appropriate
> translation between 32-bit and 64-bit time to call those syscalls (and the
> expected design is that the unsuffixed ABIs will generally be thin
> wrappers that do such translation and call the suffixed ones).

Ok, I didn't realise that this was a possible case, I will update the series.

Alistair

>
> 4. __TIMESIZE == 32 and __ASSUME_TIME64_SYSCALLS not defined means a
> 32-bit glibc port, where the unsuffixed time-related ABIs in glibc use
> 32-bit time but the Y2038 work will add corresponding suffixed ABIs (and
> _TIME_BITS headers support) that use 64-bit time - and where the minimum
> kernel version does not ensure that 64-bit-time (suffixed) syscalls are
> available.  Thus, the suffixed ABIs for 64-bit time need to have support
> for falling back to the 32-bit syscalls when the 64-bit ones are not
> available.
>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

Alistair Francis-2
In reply to this post by Joseph Myers
   On Mon, Aug 12, 2019 at 10:22 AM Joseph Myers
<[hidden email]> wrote:

>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> > index 07a51808df2..fc495b56c67 100644
> > --- a/nptl/thrd_sleep.c
> > +++ b/nptl/thrd_sleep.c
> > @@ -25,14 +25,68 @@ int
> >  thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> >  {
> >    INTERNAL_SYSCALL_DECL (err);
> > -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> > +  int ret = -1;
> > +
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                 0, time_point, remaining);
> > +#else
>
> This still has the same problem as has been explained for previous
> versions of the patch series: if time_t is 32-bit but
> __ASSUME_TIME64_SYSCALLS is defined, it is not valid to call the
> clock_nanosleep_time64 with a 32-bit timespec; you have to call it with a
> 64-bit timespec.  That is, if you call clock_nanosleep_time64 at all in
> the case where the original function was called with a 32-bit timespec
> (and I think you should do so) then you need to have conversions.

As discussed in a different thread, I will update the series to
support 32 and 64 bit time_t with __ASSUME_TIME64_SYSCALLS defined.

>
> Please fix this *throughout* the patch series *before* posting the next
> version.  That is, for every patch in the series you need to consider what
> is appropriate for systems with 32-bit time - *not* just what is
> appropriate for RV32.
>
> The pattern we have previously discussed for 64-bit time support is that
> the code should (a) define the function (__thrd_sleep_time64, say) for
> 64-bit time (which then only needs conversions in the reverse direction -
> if the 64-bit syscall is not in fact available, but the 32-bit one is),
> (b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
> round that, (c) in the appropriate internal header, has a #define of the

Doesn't having a thing wrapper around __thrd_sleep64() result in
unnecessary conversions? When __NR_clock_nanosleep_time64 is not
defined we will end up converting a 32-bit time_t to a 64-bit time_t
just to convert it back to a 32-bit time_t.

It seems simpler to me to just keep the structure here and fix the
32-bit time_t when __ASSUME_TIME64_SYSCALLS is defined. That will
probably result in a helper function for
defined(__ASSUME_TIME64_SYSCALLS) ||
__NR_clock_nanosleep_time64 as they are very similar.

> name such as __thrd_sleep_time64 to the name such as thrd_sleep, in the
> case where __TIMESIZE == 64 and thus no wrapper is needed.
>
> > +# ifdef __NR_clock_nanosleep_time64
> > +#  if __TIMESIZE == 64
> > +  long int ret_64;
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                    0, time_point, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    ret = ret_64;
> > +#  else
> > +  timespec64 ts64;
> > +
> > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > +                                 CLOCK_REALTIME, 0, time_point,
> > +                                 ts64);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    {
> > +      remaining->tv_sec = ts64.tv_sec;
> > +      remaining->tv_nsec = ts64.tv_nsec;
> > +    }
>
> thrd_sleep has *two* timespec arguments, one input and one output.  It's
> not sufficient to just convert the output, as here; if you're doing
> conversions, you have to convert *both*.

Good point, I will fix this.

>
> It is valid for the output argument to be NULL.  You need to avoid writing
> to *remaining in that case.

I will fix this.

>
> Please try to test such 64-bit time changes in configurations covering as
> many of the different cases in the code as possible, both at compile time
> and at run time, and include details in the patch submissions of what
> configurations you tested (architecture, kernel headers version,
> --enable-kernel version, kernel version used when running the testsuite).
> I'd hope such testing would have shown up the issue with the output
> argument being NULL, as well as the ABI breakage.

Before I send a patch series I will run more tests.

>
> Relevant configurations should include at least (a) one that has always
> had 64-bit time, e.g. x86_64; (b) one with 32-bit time and old kernel
> headers (any kernel version at runtime); (c) one with 32-bit time and new
> kernel headers, old kernel at runtime; (d) one with 32-bit time and new
> kernel headers, new kernel at runtime but no --enable-kernel; (e) one with
> 32-bit time and new kernel at runtime and new --enable-kernel.  (New = 5.1
> or later.)  I think that's a basic minimum for testing any patches related
> to 64-bit time.  (If Florian's changes to provide syscall tables within
> glibc go in, case (b) disappears.)

Setting up all these cases will take a long time, which is why I
haven't done it yet for a RFC series.

>
> (In this case, you're also passing the struct ts64 by value to the syscall
> rather than a pointer to it.  And as this is C, not C++, I'm not sure a
> declaration "timespec64 ts64;" without "struct" would have compiled.)

Will fix.

>
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
>
> > +#if __TIMESIZE == 32
> > +struct timespec64
> > +{
> > +  long long int tv_sec;   /* Seconds.  */
> > +  long int tv_nsec;  /* Nanoseconds.  */
> > +};
>
> If we need such a structure different from the "struct __timespec64" in
> Lukasz's patches, it surely does not belong in the .c file for one
> particular function without a very detailed comment explaining exactly why
> it's so specific to that function rather than in a more generic internal
> header.

This can probably be removed then.

Alistair

>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 23/24] WIP: syscall.list: Call 64-bit versions of syscalls

Alistair Francis-2
In reply to this post by Alistair Francis
On Fri, Aug 9, 2019 at 6:04 PM Alistair Francis
<[hidden email]> wrote:

>
> Signed-off-by: Alistair Francis <[hidden email]>
> ---
>  sysdeps/unix/sysv/linux/syscalls.list | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
> index e374f97b5f8..4844d1a9a3b 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -5,7 +5,7 @@ alarm           -       alarm           i:i     alarm
>  bdflush                EXTRA   bdflush         i:ii    __compat_bdflush        bdflush@GLIBC_2.0:GLIBC_2.23
>  capget         EXTRA   capget          i:pp    capget
>  capset         EXTRA   capset          i:pp    capset
> -clock_adjtime  EXTRA   clock_adjtime   i:ip    clock_adjtime
> +clock_adjtime  EXTRA   clock_adjtime64 i:ip    clock_adjtime
>  create_module  EXTRA   create_module   3       __compat_create_module  create_module@GLIBC_2.0:GLIBC_2.23
>  delete_module  EXTRA   delete_module   3       delete_module
>  epoll_create   EXTRA   epoll_create    i:i     epoll_create
> @@ -52,7 +52,7 @@ sched_getp    -       sched_getparam  i:ip    __sched_getparam        sched_getparam
>  sched_gets     -       sched_getscheduler      i:i     __sched_getscheduler    sched_getscheduler
>  sched_primax   -       sched_get_priority_max  i:i     __sched_get_priority_max        sched_get_priority_max
>  sched_primin   -       sched_get_priority_min  i:i     __sched_get_priority_min        sched_get_priority_min
> -sched_rr_gi    -       sched_rr_get_interval   i:ip    __sched_rr_get_interval sched_rr_get_interval
> +sched_rr_gi    -       sched_rr_get_interval_time64    i:ip    __sched_rr_get_interval sched_rr_get_interval
>  sched_setp     -       sched_setparam  i:ip    __sched_setparam        sched_setparam
>  sched_sets     -       sched_setscheduler      i:iip   __sched_setscheduler    sched_setscheduler
>  sched_yield    -       sched_yield     i:      __sched_yield   sched_yield
> @@ -96,8 +96,8 @@ fremovexattr  -       fremovexattr    i:is    fremovexattr
>  mq_setattr     -       mq_getsetattr   i:ipp   mq_setattr
>
>  timerfd_create EXTRA   timerfd_create  i:ii    timerfd_create
> -timerfd_settime        EXTRA   timerfd_settime i:iipp  timerfd_settime
> -timerfd_gettime        EXTRA   timerfd_gettime i:ip    timerfd_gettime
> +timerfd_settime        EXTRA   timerfd_settime64       i:iipp  timerfd_settime
> +timerfd_gettime        EXTRA   timerfd_gettime64       i:ip    timerfd_gettime

Does anyone have ideas/opinions on how to handle this correctly?

Alistair

>
>  fanotify_init  EXTRA   fanotify_init   i:ii    fanotify_init
>
> --
> 2.22.0
>
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 23/24] WIP: syscall.list: Call 64-bit versions of syscalls

Florian Weimer-5
* Alistair Francis:

> On Fri, Aug 9, 2019 at 6:04 PM Alistair Francis
> <[hidden email]> wrote:
>>
>> Signed-off-by: Alistair Francis <[hidden email]>
>> ---
>>  sysdeps/unix/sysv/linux/syscalls.list | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
>> index e374f97b5f8..4844d1a9a3b 100644
>> --- a/sysdeps/unix/sysv/linux/syscalls.list
>> +++ b/sysdeps/unix/sysv/linux/syscalls.list
>> @@ -5,7 +5,7 @@ alarm           -       alarm           i:i     alarm
>>  bdflush                EXTRA   bdflush         i:ii    __compat_bdflush        bdflush@GLIBC_2.0:GLIBC_2.23
>>  capget         EXTRA   capget          i:pp    capget
>>  capset         EXTRA   capset          i:pp    capset
>> -clock_adjtime  EXTRA   clock_adjtime   i:ip    clock_adjtime
>> +clock_adjtime  EXTRA   clock_adjtime64 i:ip    clock_adjtime
>>  create_module  EXTRA   create_module   3       __compat_create_module  create_module@GLIBC_2.0:GLIBC_2.23
>>  delete_module  EXTRA   delete_module   3       delete_module
>>  epoll_create   EXTRA   epoll_create    i:i     epoll_create
>> @@ -52,7 +52,7 @@ sched_getp    -       sched_getparam  i:ip    __sched_getparam        sched_getparam
>>  sched_gets     -       sched_getscheduler      i:i     __sched_getscheduler    sched_getscheduler
>>  sched_primax   -       sched_get_priority_max  i:i     __sched_get_priority_max        sched_get_priority_max
>>  sched_primin   -       sched_get_priority_min  i:i     __sched_get_priority_min        sched_get_priority_min
>> -sched_rr_gi    -       sched_rr_get_interval   i:ip    __sched_rr_get_interval sched_rr_get_interval
>> +sched_rr_gi    -       sched_rr_get_interval_time64    i:ip    __sched_rr_get_interval sched_rr_get_interval
>>  sched_setp     -       sched_setparam  i:ip    __sched_setparam        sched_setparam
>>  sched_sets     -       sched_setscheduler      i:iip   __sched_setscheduler    sched_setscheduler
>>  sched_yield    -       sched_yield     i:      __sched_yield   sched_yield
>> @@ -96,8 +96,8 @@ fremovexattr  -       fremovexattr    i:is    fremovexattr
>>  mq_setattr     -       mq_getsetattr   i:ipp   mq_setattr
>>
>>  timerfd_create EXTRA   timerfd_create  i:ii    timerfd_create
>> -timerfd_settime        EXTRA   timerfd_settime i:iipp  timerfd_settime
>> -timerfd_gettime        EXTRA   timerfd_gettime i:ip    timerfd_gettime
>> +timerfd_settime        EXTRA   timerfd_settime64       i:iipp  timerfd_settime
>> +timerfd_gettime        EXTRA   timerfd_gettime64       i:ip    timerfd_gettime
>
> Does anyone have ideas/opinions on how to handle this correctly?

As in many of the other cases, you can add this to <sysdep.h>:

#define __NR_timerfd_settime __NR_timerfd_settime64
#define __NR_timerfd_gettime __NR_timerfd_gettime64

Once a second such port arrives, we can factor out these common system
call renamings into a generic-64 subdirectory.

I still think it's just wrong that the kernel doesn't provide these
names as part of the UAPI headers.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

Joseph Myers
In reply to this post by Alistair Francis-2
On Wed, 14 Aug 2019, Alistair Francis wrote:

> > The pattern we have previously discussed for 64-bit time support is that
> > the code should (a) define the function (__thrd_sleep_time64, say) for
> > 64-bit time (which then only needs conversions in the reverse direction -
> > if the 64-bit syscall is not in fact available, but the 32-bit one is),
> > (b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
> > round that, (c) in the appropriate internal header, has a #define of the
>
> Doesn't having a thing wrapper around __thrd_sleep64() result in
> unnecessary conversions? When __NR_clock_nanosleep_time64 is not

The conversions are in userspace, i.e. a few instructions.

We need a clearly defined consistent convention for the pattern used for
function implementations for 32-bit and 64-bit time, not every function
doing things in its own way.  For functions such a
pthread_mutex_timedlock, with hundreds of lines of code, thin wrappers are
clearly the only reasonably approach to avoid large amounts of code
duplication.  That in turn leads to using thin wrappers consistently
everywhere the functions are defined in C, unless there is a clear reason
for a particular function to be different.

--
Joseph S. Myers
[hidden email]
1234