This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


   On Mon, Aug 12, 2019 at 10:22 AM Joseph Myers
<joseph@codesourcery.com> 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
> joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]