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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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.

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.