Re: [PATCH 1/5] Fix {INLINE,INTERNAL}_SYSCALL macros for x32

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Jul 3, 2017 at 4:52 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 01/07/2017 13:53, H.J. Lu wrote:
>> On Tue, May 23, 2017 at 11:25 AM, Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.com>
>>>
>>> The problem for x32 is the {INTERNAL,INLINE}_SYSCALL C macros explicit
>>> cast the arguments to 'long int', thus passing as 32 bits arguments
>>> that should be passed to 64 bits.
>>>
>>> Previous x32 implementation uses the auto-generated syscalls from
>>> assembly macros (syscalls.list), so the {INTERNAL,INLINE}_SYSCALL
>>> macros are never used with 64 bit argument in x32 (which are
>>> internally broken for this ILP).
>>>
>>> To fix it I used a strategy similar to MIPS64n32 (although both
>>> ABI differs for some syscalls on how top pass 64-bits arguments)
>>> where argument types for kernel call are defined using GCC extension
>>> 'typeof' with a arithmetic operation.  This allows 64-bits arguments
>>> to be defined while 32-bits argument will still passed as 32-bits.
>>>
>>> I also cleanup the {INLINE,INTERNAL}_SYSCALL definition by defining
>>> 'inline_syscallX' instead of constructing the argument passing using
>>> macros (it adds some readability) and removed the ununsed
>>> INTERNAL_SYSCALL_NCS_TYPES define (since the patch idea is exactly to
>>> avoid requiric explicit types passing).
>>>
>>> Tested on x86_64 and x32.
>>>
>>>         * sysdeps/unix/sysv/linux/x86_64/sysdep.h
>>>         (INTERNAL_SYSCALL_NCS_TYPES): Remove define.
>>>         (LOAD_ARGS_0): Likewise.
>>>         (LOAD_ARGS_1): Likewise.
>>>         (LOAD_ARGS_2): Likewise.
>>>         (LOAD_ARGS_3): Likewise.
>>>         (LOAD_ARGS_4): Likewise.
>>>         (LOAD_ARGS_5): Likewise.
>>>         (LOAD_ARGS_6): Likewise.
>>>         (LOAD_REGS_0): Likewise.
>>>         (LOAD_REGS_1): Likewise.
>>>         (LOAD_REGS_2): Likewise.
>>>         (LOAD_REGS_3): Likewise.
>>>         (LOAD_REGS_4): Likewise.
>>>         (LOAD_REGS_5): Likewise.
>>>         (LOAD_REGS_6): Likewise.
>>>         (ASM_ARGS_0): Likewise.
>>>         (ASM_ARGS_1): Likewise.
>>>         (ASM_ARGS_2): Likewise.
>>>         (ASM_ARGS_3): Likewise.
>>>         (ASM_ARGS_4): Likewise.
>>>         (ASM_ARGS_5): Likewise.
>>>         (ASM_ARGS_6): Likewise.
>>>         (LOAD_ARGS_TYPES_1): Likewise.
>>>         (LOAD_ARGS_TYPES_2): Likewise.
>>>         (LOAD_ARGS_TYPES_3): Likewise.
>>>         (LOAD_ARGS_TYPES_4): Likewise.
>>>         (LOAD_ARGS_TYPES_5): Likewise.
>>>         (LOAD_ARGS_TYPES_6): Likewise.
>>>         (LOAD_REGS_TYPES_1): Likewise.
>>>         (LOAD_REGS_TYPES_2): Likewise.
>>>         (LOAD_REGS_TYPES_3): Likewise.
>>>         (LOAD_REGS_TYPES_4): Likewise.
>>>         (LOAD_REGS_TYPES_5): Likewise.
>>>         (LOAD_REGS_TYPES_6): Likewise.
>>>         (TYPEFY): New define.
>>>         (ARGIFY): Likewise.
>>>         (internal_syscall0): Likewise.
>>>         (internal_syscall1): Likewise.
>>>         (internal_syscall2): Likewise.
>>>         (internal_syscall3): Likewise.
>>>         (internal_syscall4): Likewise.
>>>         (internal_syscall5): Likewise.
>>>         (internal_syscall6): Likewise.
>>>         * sysdeps/unix/sysv/linux/x86_64/x32/times.c
>>>         (INTERNAL_SYSCALL_NCS): Remove define.
>>>         (internal_syscall1): Add define.
>>> ---
>>>  ChangeLog                                  |  50 ++++++
>>>  sysdeps/unix/sysv/linux/x86_64/sysdep.h    | 251 ++++++++++++++++-------------
>>>  sysdeps/unix/sysv/linux/x86_64/x32/times.c |  24 +--
>>>  3 files changed, 205 insertions(+), 120 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
>>> index 7b8bd79..6d0a6f4 100644
>>> --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
>>> +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
>>> @@ -221,33 +221,148 @@
>>>  /* Registers clobbered by syscall.  */
>>>  # define REGISTERS_CLOBBERED_BY_SYSCALL "cc", "r11", "cx"
>>>
>>> -# define INTERNAL_SYSCALL_NCS(name, err, nr, args...) \
>>> -  ({                                                                         \
>>> -    unsigned long int resultvar;                                             \
>>> -    LOAD_ARGS_##nr (args)                                                    \
>>> -    LOAD_REGS_##nr                                                           \
>>> -    asm volatile (                                                           \
>>> -    "syscall\n\t"                                                            \
>>> -    : "=a" (resultvar)                                                       \
>>> -    : "0" (name) ASM_ARGS_##nr : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);   \
>>> -    (long int) resultvar; })
>>> -# undef INTERNAL_SYSCALL
>>> -# define INTERNAL_SYSCALL(name, err, nr, args...) \
>>> -  INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, ##args)
>>> -
>>> -# define INTERNAL_SYSCALL_NCS_TYPES(name, err, nr, args...) \
>>> -  ({                                                                         \
>>> -    unsigned long int resultvar;                                             \
>>> -    LOAD_ARGS_TYPES_##nr (args)                                                      \
>>> -    LOAD_REGS_TYPES_##nr (args)                                                      \
>>> -    asm volatile (                                                           \
>>> -    "syscall\n\t"                                                            \
>>> -    : "=a" (resultvar)                                                       \
>>> -    : "0" (name) ASM_ARGS_##nr : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);   \
>>> -    (long int) resultvar; })
>>> -# undef INTERNAL_SYSCALL_TYPES
>>> -# define INTERNAL_SYSCALL_TYPES(name, err, nr, args...) \
>>> -  INTERNAL_SYSCALL_NCS_TYPES (__NR_##name, err, nr, ##args)
>>> +/* Create a variable 'name' based on type 'X' to avoid explicit types.
>>> +   This is mainly used set use 64-bits arguments in x32.   */
>>> +#define TYPEFY(X, name) __typeof__ ((X) - (X)) name
>>> +/* Explicit cast the argument to avoid integer from pointer warning on
>>> +   x32.  */
>>> +#define ARGIFY(X) ((__typeof__ ((X) - (X))) (X))
>>> +
>>
>> I am a little bit concerned about this.  The current macros cast to long int
>> by default on purpose.  64-bit return and arguments are treated as
>> special cases.   With your change, this code in loadlocale.c:
>>
>>          if (filedata != NULL)
>>             {
>>               off_t to_read = st.st_size;
>>               ssize_t nread;
>>               char *p = (char *) filedata;
>>               while (to_read > 0)
>>                 {
>>                   nread = read_not_cancel (fd, p, to_read);
>>                   if (__builtin_expect (nread, 1) <= 0)
>>                     {
>>                       free (filedata);
>>                       if (nread == 0)
>>                         __set_errno (EINVAL); /* Bizarreness going on.  */
>>                       goto puntfd;
>>                     }
>>                   p += nread;
>>
>> is generated differently.  On x32, ssize_t is 32 bits and offset is 64 bits.
>> Before your patch, to_read is passed as 32 bit value.  With your change,
>> __typeof__ ((X) - (X)) turns to_read into 64 bits.  It may be OK for this
>> particular case.  But I am not certain that your change is 100% safe in
>> all cases.
>
> I think the problem is we define 'read_not_cancel' as a macro rather than
> an static inline with expected arguments types.  For a QoI I think creating
> wrappers around it would be better.

That is fine with me.

> I would like to make the syscall internal macros to work seamlessly across
> all architectures and avoid to create specialized assembly macros for
> specific syscalls (such as lseek{64} for x32 witch I think it is not
> avoidable). Also x32 is the only architecture afaik preventing the
> syscall-cancel.h cleanup currently.

There is fine with me as long as the implementation is correct for
x32.   But the current one isn't correct for x32.

Thanks.

-- 
H.J.