This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 11/12] Linux/Alpha: don’t use timeval32 system calls.
On 22/08/2019 10:36, Zack Weinberg wrote:
> On 8/21/19 2:49 PM, Adhemerval Zanella wrote:
>
>>> +__adjtimex_tv64 (struct timex *tx)
>>> +{
>>> + return ADJTIMEX (tx);
>>> +}
>>>
>>> libc_hidden_ver (__adjtimex_tv64, __adjtimex)
>>> strong_alias (__adjtimex_tv64, __adjtimex_tv64p);
>>
>> I think you can just remove this file by adding
>>
>> adjtimex adjtime adjtimex i:p __adjtimex __adjtimex@GLIBC_2.1 adjtimex@GLIBC_2.1
>>
>> on sysdeps/unix/sysv/linux/alpha/syscalls.list.
>
> Thanks for the suggestion. I'll try it.
>
>>> + if (__adjtime (&itv64, &otv64))
>>> + return -1;
>>
>> No implicit checks.
>
> *sigh* Will change.
>
>>> +int attribute_compat_text_section
>>> +__adjtimex_tv32 (struct timex32 *tx)
>>> +{
>>> + struct timex tx64;
>>> + memset (&tx64, 0, sizeof tx64);
>>
>> Maybe struct timex tx64 = { 0 } ?
>
> I would prefer to keep parallel structure with the treatment of `tx`
> below; we cannot use an initializer for that.
Fair enough.
>
>>> + if (TV64_TO_TV32 (&tx->time, &tx64.time))
>>> + {
>>> + __set_errno (EOVERFLOW);
>>> + return -1;
>>> + }
>>
>> I am not sure it is correct to return EOVERFLOW for this pattern, because
>> the side-effects of time adjustment will be visible after the call. The
>> kernel (arch/alpha/kernel/osf_sys.c:1254) does not handle the overflow,
>> so I think we just need to mimic the kernel for this (the caller will see
>> the overflow in the returned value).
>
> This (and other comments along these lines) make me realize we need to
> have a design discussion about this patch's proposed semantics for Y2038
> overflow, separate from the line-by-line review.
>
> To facilitate this, I am going to split this patch from the larger
> series and resubmit it separately, after making some changes to the
> semantics, not necessarily the ones you asked for.
>
> (Unfortunately, because gettimeofday is one of the affected symbols, the
> larger patch series depends on this patch. So we're going to have to
> have the discussion now.)
Alright.
>
>>> +#ifndef _TV32_COMPAT_H
>>> +#define _TV32_COMPAT_H 1
>>
>> I personally prefer to prefix arch-specific header with the architectures
>> initial to allow include-next (although this header specific does not
>> actually require it).
>
> I didn't do that in this case because I am expecting that this header
> will be moved up to sysdeps/unix/sysv/linux or maybe even all the way to
> sysdeps/posix as part of the larger Y2038 project. We want to have
> consistent overflow behavior on all architectures, after all. The only
> reason not to make it arch-generic right now is I haven't checked that
> other 32-bit architectures' structure layouts match the ones used for
> Linux/Alpha.
Alright.
>
>>> +struct rusage32
>>> +{
>>> + struct timeval32 ru_utime; /* user time used */
>>> + struct timeval32 ru_stime; /* system time used */
>>> + long ru_maxrss; /* maximum resident set size */
>>> + long ru_ixrss; /* integral shared memory size */
>>> + long ru_idrss; /* integral unshared data size */
>>> + long ru_isrss; /* integral unshared stack size */
>>> + long ru_minflt; /* page reclaims */
>>> + long ru_majflt; /* page faults */
>>> + long ru_nswap; /* swaps */
>>> + long ru_inblock; /* block input operations */
>>> + long ru_oublock; /* block output operations */
>>> + long ru_msgsnd; /* messages sent */
>>> + long ru_msgrcv; /* messages received */
>>> + long ru_nsignals; /* signals received */
>>> + long ru_nvcsw; /* voluntary context switches */
>>> + long ru_nivcsw; /* involuntary " */
>>> +};
>>
>> s/long/long int/
>
> Must we? That will make it harder for people in the future to check
> whether the structure matches the kernel headers' version.
This is the feedback I got from Florian on my 'Refactor sigcontextinfo.h'
patch where I also copied some struct definition from kernel. I think
we should follow GNU code convention even for such cases.
>
>>> +/* Conversion functions. If the seconds field of a timeval32 would
>>> + overflow, they write { INT32_MAX, 0 } to the output and return -1;
>>> + otherwise they return 0. */
>>> +
>>> +__extern_always_inline void
>>> +TV32_TO_TV64 (struct timeval *restrict tv64,
>>> + const struct timeval32 *restrict tv32)
>>> +{
>>> + tv64->tv_sec = tv32->tv_sec;
>>> + tv64->tv_usec = tv32->tv_usec;
>>> +}
>>
>> I think since it is an internal header and we explict build glibc with gnu11,
>> use static inline should be suffice instead of __extern_always_inline. Also,
>> although it is not a rule, glibc uses capitalized names for macros definitions
>> and default names for static inline functions. The implementation looks ok.
>
> I'm using capitalized names to match the convention established by
> <sys/time.h>'s TIMEVAL_TO_TIMESPEC and friends.
>
> I will switch to static inline.
>
>> Use bool/_Bool so there is no need to use explicit check for the function
>> use. Same for TS64_TO_TV32 and RUSAGE64_TO_RUSAGE32.
>
> I will also make this change.
>
> zw
>