[committed] libstdc++: Use custom timespec in system calls [PR 93421]

[Jonathan Wakely]

On 14/11/20 14:23 +0000, Jonathan Wakely via Libstdc++ wrote:
>On Sat, 14 Nov 2020, 13:30 Mike Crowe via Libstdc++, <libstdc++@gcc.gnu.org>
>wrote:
>
>> On Saturday 14 November 2020 at 00:17:59 +0000, Jonathan Wakely via
>> Libstdc++ wrote:
>> > On 32-bit targets where userspace has switched to 64-bit time_t, we
>> > cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
>> > the userspace definition of struct timespec will not match what the
>> > kernel expects.
>> >
>> > We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
>> > macros to imply that userspace *might* have switched to the new timespec
>> > definition.  This is a conservative assumption. It's possible that the
>> > new syscall numbers are defined in the libc headers but that timespec
>> > hasn't been updated yet (as is the case for glibc currently). But using
>> > the alternative struct with two longs is still OK, it's just redundant
>> > if userspace timespec still uses a 32-bit time_t.
>> >
>> > We also check that SYS_futex_time64 != SYS_futex so that we don't try
>> > to use a 32-bit tv_sec on modern targets that only support the 64-bit
>> > system calls and define the old macro to the same value as the new one.
>> >
>> > We could possibly check #ifdef __USE_TIME_BITS64 to see whether
>> > userspace has actually been updated, but it's not clear if user code
>> > is meant to inspect that or if it's only for libc internal use.
>> >
>> > libstdc++-v3/ChangeLog:
>> >
>> >       PR libstdc++/93421
>> >       * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
>> >       (syscall_timespec): Define a type suitable for SYS_clock_gettime
>> >       calls.
>> >       (system_clock::now(), steady_clock::now()): Use syscall_timespec
>> >       instead of timespec.
>> >       * src/c++11/futex.cc (syscall_timespec): Define a type suitable
>> >       for SYS_futex and SYS_clock_gettime calls.
>> >       (relative_timespec): Use syscall_timespec instead of timespec.
>> >       (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
>> >       (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
>> >       Likewise.
>> >
>> > Tested x86_64-linux, -m32 too. Committed to trunk.
>> >
>>
>> > commit 4d039cb9a1d0ffc6910fe09b726c3561b64527dc
>> > Author: Jonathan Wakely <jwakely@redhat.com>
>> > Date:   Thu Sep 24 17:35:52 2020
>> >
>> >     libstdc++: Use custom timespec in system calls [PR 93421]
>> >
>> >     On 32-bit targets where userspace has switched to 64-bit time_t, we
>> >     cannot pass struct timespec to SYS_futex or SYS_clock_gettime,
>> because
>> >     the userspace definition of struct timespec will not match what the
>> >     kernel expects.
>> >
>> >     We use the existence of the SYS_futex_time64 or
>> SYS_clock_gettime_time64
>> >     macros to imply that userspace *might* have switched to the new
>> timespec
>> >     definition.  This is a conservative assumption. It's possible that
>> the
>> >     new syscall numbers are defined in the libc headers but that timespec
>> >     hasn't been updated yet (as is the case for glibc currently). But
>> using
>> >     the alternative struct with two longs is still OK, it's just
>> redundant
>> >     if userspace timespec still uses a 32-bit time_t.
>> >
>> >     We also check that SYS_futex_time64 != SYS_futex so that we don't try
>> >     to use a 32-bit tv_sec on modern targets that only support the 64-bit
>> >     system calls and define the old macro to the same value as the new
>> one.
>> >
>> >     We could possibly check #ifdef __USE_TIME_BITS64 to see whether
>> >     userspace has actually been updated, but it's not clear if user code
>> >     is meant to inspect that or if it's only for libc internal use.
>>
>> Presumably this is change is only good for the short term? We really want
>> to be calling the 64-bit time_t versions of SYS_futex and SYS_clock_gettime
>> passing 64-bit struct timespec so that this code continues to work
>> correctly after 2038 (for CLOCK_REALTIME) or if someone is unlucky enough
>> to have a system uptime of over 68 years (for CLOCK_MONOTONIC.) Perhaps
>> that's part of the post-GCC11 work that you plan to do.
>>
>
>Right. This is definitely a short term solution, but I ran out of time to
>do something better for GCC 11.

We can still do a bit better than that patch though, which was just
broken (it's SYS_clock_gettime64 not SYS_clock_gettime_time64 for a
start).

This partially reverts it, to remove the conditional compilation
related to SYS_clock_gettime64, because that's almost certainly never
going to be needed.

Tested x86_64-linux, with glibc 2.12 and 2.31, committed to trunk.


>> > @@ -195,7 +205,7 @@ namespace
>> >           if (__s.count() < 0) [[unlikely]]
>> >             return false;
>> >
>> > -         struct timespec rt;
>> > +         syscall_timespec rt;
>> >           if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
>> >             rt.tv_sec = __int_traits<time_t>::__max;
>>
>> Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
>> yet syscall_timespec is using 32-bit long?
>>
>
>Ah yes. Maybe decltype(rt.tv_sec).

I'll fix that in the next patch.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 6806 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20201118/bb78e1ce/attachment.bin>