From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: "H.J. Lu" <hjl.tools@gmail.com>
Cc: GNU C Library <libc-alpha@sourceware.org>,
Adhemerval Zanella <adhemerval.zanella@linaro.com>
Subject: Re: [PATCH 1/5] Fix {INLINE,INTERNAL}_SYSCALL macros for x32
Date: Mon, 03 Jul 2017 11:53:00 -0000 [thread overview]
Message-ID: <044c5243-5e6c-69a7-b92e-c6d290cd9ce6@linaro.org> (raw)
In-Reply-To: <CAMe9rOo0-4gmb3jnOzFNCyppu=ncd9UFnRsey3cNWvEM6u2LPA@mail.gmail.com>
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.
next prev parent reply other threads:[~2017-07-03 11:53 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-23 18:26 [PATCH 0/5] Remove sysdep-cancel assembly macro Adhemerval Zanella
2017-05-23 18:26 ` [PATCH 3/5] linux: Consolidate sync_file_range implementation Adhemerval Zanella
2017-06-15 18:46 ` Adhemerval Zanella
2017-05-23 18:26 ` [PATCH 5/5] Remove cancellation support for syscall generation Adhemerval Zanella
2017-05-23 18:26 ` [PATCH 2/5] Remove p{read,write}{v} and fallocate from x86 auto-generation list Adhemerval Zanella
2017-06-29 14:15 ` Adhemerval Zanella
2017-05-23 18:26 ` [PATCH 1/5] Fix {INLINE,INTERNAL}_SYSCALL macros for x32 Adhemerval Zanella
2017-06-29 14:15 ` Adhemerval Zanella
2017-06-29 18:51 ` Florian Weimer
2017-06-30 15:03 ` Adhemerval Zanella
2017-07-01 16:53 ` H.J. Lu
2017-07-03 11:53 ` Adhemerval Zanella [this message]
2017-07-03 19:44 ` H.J. Lu
2017-05-23 18:26 ` [PATCH 4/5] Consolidate Linux fcntl implementation Adhemerval Zanella
2017-06-29 14:15 ` Adhemerval Zanella
2017-06-29 15:01 ` Andreas Schwab
2017-06-02 12:51 ` [PATCH 0/5] Remove sysdep-cancel assembly macro Adhemerval Zanella
2017-06-13 14:31 ` Adhemerval Zanella
2017-06-29 14:16 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=044c5243-5e6c-69a7-b92e-c6d290cd9ce6@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=adhemerval.zanella@linaro.com \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).