[PATCH v7.1 07/13] ARC: Linux Syscall Interface

[Vineet Gupta]

On 7/6/20 6:20 AM, Adhemerval Zanella via Libc-alpha wrote:
>>>> diff --git a/sysdeps/unix/sysv/linux/arc/clone.S b/sysdeps/unix/sysv/linux/arc/clone.S
>>
>>>> diff --git a/sysdeps/unix/sysv/linux/arc/fixup-asm-unistd.h b/sysdeps/unix/sysv/linux/arc/fixup-asm-unistd.h
>>
>>>> +
>>>> +/* Adjustments to ARC asm-generic syscall ABI (3.9 kernel) for 64-bit time_t
>>>> +   support.  */
>>>> +
>>>> +/* fstat64 and fstatat64 need to be replaced with statx.  */
>>>> +
>>>> +#undef __NR_fstat64
>>>> +#undef __NR_fstatat64
>>
>> This is certainly needed as they are present in ARC arch-syscall.h but we need to
>> use statx.
>>
>>>> +/* Replace all other 32-bit time syscalls with 64-bit variants.  */
>>>> +
>>>> +# undef __NR_clock_adjtime
>>>> +# undef __NR_clock_getres
>>>> +# undef __NR_futex
>>>> +# undef __NR_mq_timedreceive
>>>> +# undef __NR_mq_timedsend
>>>> +# undef __NR_ppoll
>>>> +# undef __NR_pselect6
>>>> +# undef __NR_recvmmsg
>>>> +# undef __NR_rt_sigtimedwait
>>>> +# undef __NR_sched_rr_get_interval
>>>> +# undef __NR_semtimedop
>>>> +# undef __NR_timerfd_settime
>>>> +# undef __NR_timerfd_gettime
>>>> +# undef __NR_utimensat
>>>
>>> I am trying to understand why these are required since arc does not define 
>>> them in arch-syscall.h.
>>
>> arch-syscall.h doesn't define them precisely due to these being here. When
>> update-syscalls is run, the 32-bit syscalls are generated for ARC (since kernel
>> ABI provides these because that was v3.9 circa 2013). Adding them
>> fixup-asm-unistd.h removes them (perhaps I need to add this in changelog to
>> clarify - atleast for myself).
>>
>>> And the generic implementation should handle the time64 variant.  If they
>>> are not this is something we need to handle it.
>>
>> At the time we we doing this, arch-syscall.h generation was not yet in place,
>> however I tried to undef in generic/sysdep.h for TIMESIZE==64. However I was asked
>> me to add this to ARC specific fixup-asm-unistd.h
>> https://sourceware.org/pipermail/libc-alpha/2020-March/112395.html
>> https://sourceware.org/pipermail/libc-alpha/2020-April/112909.html
> 
> My confusion here, I forgot that this header is only used glibcsyscalls.py
> to actually generate arch-syscall.h.
> 
> You changes does look correct.

Actually we can add a few more entries here which have 64-bit variants.

+# undef __NR_clock_gettime
+# undef __NR_clock_nanosleep
+# undef __NR_clock_settime
+# undef __NR_timer_gettime
+# undef __NR_timer_settime


>>>> diff --git a/sysdeps/unix/sysv/linux/arc/kernel_stat.h b/sysdeps/unix/sysv/linux/arc/kernel_stat.h

>> This specific one is actually dead code. I did post a patch to this effect and
>> followed up with supporting data that enabling it on 64-bit arches doesn't lead to
>> any changes in generated code.
>>
>> https://sourceware.org/pipermail/libc-alpha/2020-February/111259.html
>> https://sourceware.org/pipermail/libc-alpha/2020-June/115217.html
>>
> 
> Ack.

Thx. I'll repost this after things settle at bit.

>>>> diff --git a/sysdeps/unix/sysv/linux/arc/sysdep.h b/sysdeps/unix/sysv/linux/arc/sysdep.h
>>
>> ...
>>
>>>> +/* 32-bit time syscalls are not available, but the redefines allow generic
>>>> +   wrappers to work.  */
>>>> +#define __NR_clock_adjtime	__NR_clock_adjtime64
>>>> +#define __NR_clock_getres	__NR_clock_getres_time64
>>>> +#define __NR_futex		__NR_futex_time64
>>>> +#define __NR_mq_timedreceive	__NR_mq_timedreceive_time64
>>>> +#define __NR_mq_timedsend	__NR_mq_timedsend_time64
>>>> +#define __NR_ppoll		__NR_ppoll_time64
>>>> +#define __NR_pselect6		__NR_pselect6_time64
>>>> +#define __NR_recvmmsg		__NR_recvmmsg_time64
>>>> +#define __NR_rt_sigtimedwait	__NR_rt_sigtimedwait_time64
>>>> +#define __NR_sched_rr_get_interval	__NR_sched_rr_get_interval_time64
>>>> +#define __NR_semtimedop		__NR_semtimedop_time64
>>>> +#define __NR_timerfd_gettime	__NR_timerfd_gettime64
>>>> +#define __NR_timerfd_settime	__NR_timerfd_settime64
>>>> +#define __NR_utimensat		__NR_utimensat_time64
>>>
>>> As for the fixup-asm-unistd.h, the generic implementation should handle it
>>> without the requirement of the ABI to add such tricks.
>>
>> fixup-asm-unistd.h is different, but this could be avoided. I know for sure that
>> ll code literally expects __NR_futex (atleast used to). But I can remove this and
>> see what comes out.
>>
>>>
>>> However it seems that we are still missing support for pselect 
>>> (__NR_pselect6_time64), recvmmsg (__NR_recvmmsg_time64), sigtimedwait 
>>> (__NR_rt_sigtimedwait_time64), and semtimeop (__NR_semtimedop_time64).
>>>
>>> I think we can add the redefine hack only the aforementioned symbols for
>>> now and removed them once we implement the y2038 support on such symbols
>>> (since the expected ABI won't change for ARC, only for old ABIs with
>>> 32 time_t support).
>>
>> Sorry /me horribly confused here.
> 
> Sorry for the confusion, I meant that some of these re-defines are superfluous 
> and I would like to have the minimum required re-define to enable the ARC 
> support, 

Right. The generic code needs a bit more work to eliminate the redefines altogether.

1. Following is not needed

-#define __NR_clock_adjtime     __NR_clock_adjtime64
-#define __NR_sched_rr_get_interval     __NR_sched_rr_get_interval_time64
-#define __NR_mq_timedreceive   __NR_mq_timedreceive_time64
-#define __NR_mq_timedsend      __NR_mq_timedsend_time64
-#define __NR_timerfd_gettime   __NR_timerfd_gettime64
-#define __NR_timerfd_settime   __NR_timerfd_settime64

2. The minimum list needed for ARC (with annotations as to which generic file
needs fixing).

/* Fix sysdeps/unix/sysv/linux/clock_getcpuclockid.c.  */
#define __NR_clock_getres      __NR_clock_getres_time64
/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
#define __NR_futex             __NR_futex_time64
/* Fix sysdeps/unix/sysv/linux/pause.c.  */
#define __NR_ppoll             __NR_ppoll_time64
/* Fix sysdeps/unix/sysv/linux/select.c.  */
#define __NR_pselect6          __NR_pselect6_time64
/* Fix sysdeps/unix/sysv/linux/recvmmsg.c.  */
#define __NR_recvmmsg          __NR_recvmmsg_time64
/* Fix sysdeps/unix/sysv/linux/sigtimedwait.c.  */
#define __NR_rt_sigtimedwait   __NR_rt_sigtimedwait_time64
/* Fix sysdeps/unix/sysv/linux/semtimedop.c.  */
#define __NR_semtimedop                __NR_semtimedop_time64
/* Hack sysdeps/unix/sysv/linux/generic/utimes.c (need linux/utimes.c).  */
#define __NR_utimensat         __NR_utimensat_time64


> so we can cleanup these later once we enable time64_t support on 
> old ABIs as well.

IMO the cleanup applies to new ABIs too as generic code should handle those cases
w/o these workarounds. But that would delay things further for new ports so I
suggest we keep the workarounds and clean things up going fwd.

BTW, if one were to actually go about fixing those, whats the best approach.
Consider the simplest case pause(). For !__NR_pause do we replicate the code for
ppoll/ppoll64 handling or simply just call ppoll(). Later has a function call
overhead) ? Or there is a paradigm to use __syscallxxx_helper() although that
still has a function call overhead.

Actually the pause case is really simple as there are no args, so just redefine
__NR_xxx trick should suffice w/o going into all the explicit
interworking/conversion etc.

__libc_pause (void)
{
#ifdef __NR_pause
  return SYSCALL_CANCEL (pause);
#else

  return SYSCALL_CANCEL (ppoll, NULL, 0, NULL, NULL);
#endif


>>>> +#undef SYS_ify
>>>> +#define SYS_ify(syscall_name)   __NR_##syscall_name
>>>
>>> The code mixes __NR_* and SYS_ify macro. This macro is really superflous
>>> and I am preparing some patches to cleanup this up along with C asm
>>> macros to generate syscall.  So I would suggest to just use the __NR_*
>>> way and drop this definition.
>>
>> I don't mind, but it seems that the wrapper was a simply way to avoid open-coding
>> the macro concatenation. e.g.
>>
>> # define DO_CALL(syscall_name, args)                    \
>> -    mov    r8, SYS_ify (syscall_name)  ASM_LINE_SEP    \
>> +    mov    r8, __NR__##syscall_name  ASM_LINE_SEP    \
>>      ARC_TRAP_INSN
>>
> 
> My understand was in fact parametrized way to define syscall numbers when
> glibc added support to future multiple Unix implementation (which never
> actually happened).  I don't have a strong opinion here in fact, any is
> fine in the end.

I open-coded the 2 calls to SYS_ify() in ARC code and deleted SYS_ify()...

... and 2 hrs later, after mysterious build failures, find that SYS_ify() needs to
be retained (even if not used in ARC port) it is expected/used by generic
make-syscalls.h :-(