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 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.
>> + 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.)
>> +#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.
>> +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.
>> +/* 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