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

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

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

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.

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

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

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

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