Re: [PATCH v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

[Stepan Golosunov <stepan at golosunov dot pp dot ru>]

02.05.2019 в 10:51:08 +0200 Lukasz Majewski написал:
> Hi Stepan,
> 
> > 30.04.2019 в 11:05:05 +0200 Lukasz Majewski написал:
> > > IMHO, the abstraction would be:
> > > 
> > > 1. The __ASSUME_64BIT_TIME is _never_ defined for 64 bit native
> > > systems
> > > 
> > > 2. It is defined by default in:
> > > sysdeps/unix/sysv/linux/kernel-features.h for 32 bit systems (and
> > > the actual presence of the syscall is decided upon definitions of
> > > __NR_xxx* (i.e. # ifdef __NR_clock_settime64).  
> > 
> > I think that __NR_clock_settime64 should be used unconditionally when
> > __ASSUME_64BIT_TIME is defined.
> 
> Could you clarify it a bit?

I meant something like this:

int
__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
{
#ifdef __ASSUME_64BIT_TIME
  return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
#else
…

But I see now that most of the existing code would just miscompile in
cases where __ASSUME_* is defined while corresponding __NR_* is not.

> In the code as proposed in:
> https://patchwork.ozlabs.org/patch/1092583/
> 
> The call to clock_settime64 is protected with # ifdef
> __NR_clock_settime64 - otherwise we do a fallback to (32 bit)
> clock_settime.
> 
> Moreover, the # ifdef __ASSUME_64BIT_TIME provides a fallback path if
> kernel version is older than 5.1.

The fallback would be wrong in cases where __NR_clock_settime is not
defined or is not 32-bit.

> > > As those syscalls are provided on almost every 32 bit system now
> > > (5.1-rc6):
> > > git grep -n "clock_settime64"
> > > 
> > > gives support for: arm, arm64 (compat mode), m68k, microblaze, mips,
> > > parisc, powerpc, s390, sh, sparc, x86, xtensa
> > > 
> > > So it would be reasonable to just add this __ASSUME definition code
> > > to sysdeps/unix/sysv/linux/kernel-features.h and #undef it for
> > > architectures not supporting it (i.e. c-sky and riscv).  
> > 
> > I believe that the only 32-bit architecture without
> > __NR_clock_settime64 is x32. 
> 
> Ok, I see. 
> 
> Please correct me - would it be feasible to just #undef
> __ASSYME_TIME64_SYSCALLS for x32 ?

You'll need to know whether to use __NR_clock_settime64 or
__NR_clock_settime in cases where __TIMESIZE == 64 and
__WORDSIZE == 32.

One way would be by defining __ASSUME_TIME64_SYSCALLS unconditionally
on x32 and then defining __NR_clock_settime64 to __NR_clock_settime
when __ASSUME_TIME64_SYSCALLS is defined while __NR_clock_settime64
isn't.

> > While newer 32-bit architectures like
> > riscv do not have __NR_clock_settime:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4c08b9776b392e20efc6198ebe1bc8ec1911d9b
> 
> Then it shall use clock_settime64 from the outset if support added.

It probably should have __TIMESIZE == 64 though.