This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC v4 02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
- From: Joseph Myers <joseph at codesourcery dot com>
- To: Alistair Francis <alistair dot francis at wdc dot com>
- Cc: <libc-alpha at sourceware dot org>, <arnd at arndb dot de>, <adhemerval dot zanella at linaro dot org>, <fweimer at redhat dot com>, <palmer at sifive dot com>, <macro at wdc dot com>, <zongbox at gmail dot com>, <alistair23 at gmail dot com>
- Date: Mon, 12 Aug 2019 17:22:22 +0000
- Subject: Re: [RFC v4 02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
- Ironport-sdr: tgrDiufl5g0zSmXsRss9WS81n7k4L/Yk/Wq1OFP1gfmbtaMEdL6Fpg+sWcIS5Kf9S7r5yhJmXF yfhwF59b5w2lTgCWAHReHfL1cMndNWndSEanFtH79UdPtyKHIcHhINxe7OjG7UXfXgv2dwo73Y 40HySXSSzUnxgtVl09pgTluCNbEyslZiHukPBeKvSYOQuL/bCn8qmepp6UGsfvBKchkPejc+ff fm06L0CdPwir2sVFW4sE5WKfGfMiezpGjov3fsdv3AKbC/5FPM9DBsp/asBzXHlJUpPtJVyeRx 9xY=
- Ironport-sdr: OFbsIPQNddhVrOlV2EE/43MekdlYBK+RMSEoVmlKo7pLEhEIKG+EeW+k2mHSP3jnnSrVHVDMJx dENhP/11CU7UIVG7GTiLgLkz0Bz4wkV0jVUcKMknrVWUXLOR2zfVcGjDdq7UZNevg/imQKzY/0 QThnWlk6F5LFfU88FOdztGogbLWzKw4257VINe0f02qXWZ3lzBxhe/MYuhP/dZmlPJYXW1QzQf ECXVV1RuPSLfL5I3F6ugCfssL/eFmpvvYoNkV7/DqfHksblV1syGgkSDiI1a6HNaE6pjmlpm0t NEY=
- References: <cover.1565398513.git.alistair.francis@wdc.com> <52963d258d46a3ab7eb045b4f1633ac09a5e46b4.1565398513.git.alistair.francis@wdc.com>
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