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

[Adhemerval Zanella]

On 04/07/2020 00:54, Vineet Gupta wrote:
> On 7/2/20 7:47 PM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 30/06/2020 21:08, Vineet Gupta via Libc-alpha wrote:
>>> ---
>>>    Changes since v7:
>>>      - used long int (iso int) in sysdep.h/syscall_error for handling registers
>>
>> Patch looks ok, but I have question on the __NR_* undef/define. 
>>
> 
>>> diff --git a/sysdeps/unix/sysv/linux/arc/arch-syscall.h b/sysdeps/unix/sysv/linux/arc/arch-syscall.h
> ...
>>> +#define __NR_write 64
>>> +#define __NR_writev 66
>>
>> Looks good. As side note I think an improvement would be to add
>> an annotation on update-syscall-lists.py to specify which kernel
>> version it used.
> 
> Very good idea indeed. I've asked myself the very question atleast once.
> 
> 
>>> diff --git a/sysdeps/unix/sysv/linux/arc/clone.S b/sysdeps/unix/sysv/linux/arc/clone.S
> ..
>>> +
>>> +	; adjust libc args for syscall
>>
>> Use the C comment style for constency with rest of the file.
> 
> Done.
> 
>>> 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.

> 
>>> diff --git a/sysdeps/unix/sysv/linux/arc/kernel_stat.h b/sysdeps/unix/sysv/linux/arc/kernel_stat.h
> ...
>>> +#define STATFS_IS_STATFS64 0
>>
>> Ok.
> 
> 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.

> 
>>> 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, so we can cleanup these later once we enable time64_t support on 
old ABIs as well.

(ignore my comment about fixup-asm-unistd.h, it is related to my confusion
about its internal usage).

> 
>>> +
>>> +#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.