From patchwork Tue Jul 12 15:19:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 13747 Received: (qmail 129575 invoked by alias); 12 Jul 2016 15:19:51 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 129275 invoked by uid 89); 12 Jul 2016 15:19:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=standing, 2427 X-HELO: mail-lf0-f44.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=b+TI/0VvgC/p5lbqedbcV9Innmc3+9QguRY11PdCLqs=; b=f8O1Du/k8V5J13vmHW1sXwLYQ6iR1i3o88Ras1Q33CzR2vixm9X8fU2yQd5DUCFXnX VbPs/9+lw4yJnBTHewOApeEISbc72PV1LLmmKt1FcfZiMerFnXBQP6jZ4v/sjTS9EIn/ CAl3yjolsXcdeFx6W34yR5NS3U6Jb9b0cllFyiz6QDyseSKfEtpVIrwODIqcShfhXmws 1ZVmc3AyFCN/atbzm6/Q09kJ+JkZ3JcJEgpwG1N3MnMjeSpnbhJMp05gjNfz+rvNTNKy 9zs2Jq41ftcoT1zq6uXJXiv1FLVaWpJqn4r0OuzkDtArDB49Ol1D+MOoVIom7WaMc7iU AXEw== X-Gm-Message-State: ALyK8tKl9BB9Osr6Wp9v5aCNjhqAbFrykiazEDkmDb/H+4M/tncWOdzbYJo1h+RIHVFOPeR7 X-Received: by 10.25.214.166 with SMTP id p38mr1608251lfi.168.1468336774728; Tue, 12 Jul 2016 08:19:34 -0700 (PDT) Subject: Re: [PATCH] x86-64: Add p{read,write}[v]64 to syscalls.list [BZ #20348] To: "H.J. Lu" References: <20160712132628.GA7361@intel.com> <5784F8D4.4000402@linaro.org> Cc: GNU C Library From: Adhemerval Zanella Message-ID: <57850A82.3080000@linaro.org> Date: Tue, 12 Jul 2016 16:19:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: On 12/07/2016 16:03, H.J. Lu wrote: > On Tue, Jul 12, 2016 at 7:04 AM, Adhemerval Zanella > wrote: >> >> >> On 12/07/2016 14:26, H.J. Lu wrote: >>> 64-bit off_t in pread64, preadv, pwrite64 and pwritev syscalls is pased >>> in one 64-bit register for both x32 and x86-64. Since the inline >>> asm statement only passes long, which is 32-bit for x32, in registers, >>> 64-bit off_t is truncated to 32-bit on x32. Since __ASSUME_PREADV and >>> __ASSUME_PWRITEV are defined unconditionally, these syscalls can be >>> implemented in syscalls.list to pass 64-bit off_t in one 64-bit register. >>> >>> Tested on x86-64 and x32 with off_t > 4GB on pread64/pwrite64 and >>> preadv64/pwritev64. >>> >>> OK for master? >>> >>> H.J. >>> --- >>> [BZ #20348] >>> * sysdeps/unix/sysv/linux/x86_64/syscalls.list: Add pread64, >>> preadv64, pwrite64 and pwritev64. >>> --- >>> sysdeps/unix/sysv/linux/x86_64/syscalls.list | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/sysdeps/unix/sysv/linux/x86_64/syscalls.list b/sysdeps/unix/sysv/linux/x86_64/syscalls.list >>> index d09d101..bcf6370 100644 >>> --- a/sysdeps/unix/sysv/linux/x86_64/syscalls.list >>> +++ b/sysdeps/unix/sysv/linux/x86_64/syscalls.list >>> @@ -6,6 +6,10 @@ msgctl - msgctl i:iip __msgctl msgctl >>> msgget - msgget i:ii __msgget msgget >>> msgrcv - msgrcv Ci:ibnii __msgrcv msgrcv >>> msgsnd - msgsnd Ci:ibni __msgsnd msgsnd >>> +pread64 - pread64 Ci:ipii __libc_pread __libc_pread64 __pread64 pread64 __pread pread >>> +preadv64 - preadv Ci:ipii preadv64 preadv >>> +pwrite64 - pwrite64 Ci:ipii __libc_pwrite __pwrite64 pwrite64 __pwrite pwrite >>> +pwritev64 - pwritev Ci:ipii pwritev64 pwritev >>> shmat - shmat i:ipi __shmat shmat >>> shmctl - shmctl i:iip __shmctl shmctl >>> shmdt - shmdt i:s __shmdt shmdt >>> >> >> This is pretty much what I suggested [1] with the difference that my >> idea is to re-add the auto-generated wrappers just for x32. I would >> prefer to keep x86_64 continue to use default implementation and >> work on fix {INLINE,INTERNAL}_SYSCALL to work with 64-bit arguments >> in x32. > > syscalls.list is the preferred way to implement a system call, not > {INLINE,INTERNAL}_SYSCALL. There is no reason only to do it > for x32. As for {INLINE,INTERNAL}_SYSCALL with 64-bit argument, > they are only used in p{read,write}[v]64 and it is incorrect to pass long > as 64-bit integer to x32 syscall if the argument is long or pointer. The idea I am trying to push with all these consolidation are twofold: 1. Remove the complexity implementation files and way to call syscalls inside GLIBC and make easier to implement new ports 2. Remove the redundant sysdep-cancel.h requirement for each port which basically implementations pic/nopic function calls in assembly. This is also remove implementation complexity and make easier for new port implementation. Also, for 2. it also helps the long standing pthread cancellation (bz#12683) by focusing all cancellation calls in only one implementation. I do get the idea the auto-generation call is currently preferred way to implementation syscalls, but I think for *cancellable* way we should push to implement using SYSCALL_CANCEL (which is in turn based on INTERNAL_SYSCALL). > >> Also, I think we should remove the first try to fix LO_HI_LONG [2], >> since 64 bits argument does not work correct in x32 anyway. > > I think LO_HI_LONG should be defined only if __WORDSIZE != 64 > and p{read,write}[v]64 should be added to wordsize-64/syscalls.list. Indeed this is something I will get back now that I see x32 fails with current implementation. I got the wrong idea all ILP32 would use the compat code (as MIPS64n64). About the patch, based on current timeframe I think your solution is the safest one for x86. However I do would like to re-enable it on x86, including x32, when 2.25 opens. The patch below changes slight on how {INLINE,INTERNAL}_SYSCALL works on x32: int/long/pointer should be passed as before and uint64_t arguments should be passed as register all well without casting. If you have time I would like to check if it would be acceptable for 2.25. It shows no regression on x32, including the tst-preadwrite{64} testcase you sent earlier: diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c index 1419baf..3739433 100644 --- a/nptl/pthread_cancel.c +++ b/nptl/pthread_cancel.c @@ -75,8 +75,9 @@ pthread_cancel (pthread_t th) a signal handler. But this is no allowed, pthread_cancel is not guaranteed to be async-safe. */ int val; + pid_t tid = pd->tid; val = INTERNAL_SYSCALL (tgkill, err, 3, - THREAD_GETMEM (THREAD_SELF, pid), pd->tid, + THREAD_GETMEM (THREAD_SELF, pid), tid, SIGCANCEL); if (INTERNAL_SYSCALL_ERROR_P (val, err)) diff --git a/sysdeps/unix/sysv/linux/dl-origin.c b/sysdeps/unix/sysv/linux/dl-origin.c index a560203..bd65c7d 100644 --- a/sysdeps/unix/sysv/linux/dl-origin.c +++ b/sysdeps/unix/sysv/linux/dl-origin.c @@ -31,17 +31,26 @@ the path of the application from the /proc/self/exe symlink. Try this first and fall back on the generic method if necessary. */ +static inline ssize_t +_readlink (const char *pathname, char *buf, size_t bufsize) +{ + INTERNAL_SYSCALL_DECL (err); + + ssize_t ret = INTERNAL_SYSCALL (readlink, err, 3, pathname, buf, bufsize); + if (INTERNAL_SYSCALL_ERROR_P (ret, err)) + return -1; + return ret; +} + const char * _dl_get_origin (void) { char linkval[PATH_MAX]; char *result; - int len; - INTERNAL_SYSCALL_DECL (err); + ssize_t len; - len = INTERNAL_SYSCALL (readlink, err, 3, "/proc/self/exe", linkval, - sizeof (linkval)); - if (! INTERNAL_SYSCALL_ERROR_P (len, err) && len > 0 && linkval[0] != '[') + len = _readlink ("/proc/self/exe", linkval, sizeof linkval); + if (len > 0 && linkval[0] != '[') { /* We can use this value. */ assert (linkval[0] == '/'); diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h index d23136d..0fc4856 100644 --- a/sysdeps/unix/sysv/linux/not-cancel.h +++ b/sysdeps/unix/sysv/linux/not-cancel.h @@ -24,27 +24,42 @@ #include #include #include +#include +#include +#include +#include /* Uncancelable open. */ +static inline int +open_not_cancel (const char *name, int flags, int mode) +{ #ifdef __NR_open -# define open_not_cancel(name, flags, mode) \ - INLINE_SYSCALL (open, 3, name, flags, mode) -# define open_not_cancel_2(name, flags) \ - INLINE_SYSCALL (open, 2, name, flags) + return INLINE_SYSCALL (open, 3, name, flags, mode); #else -# define open_not_cancel(name, flags, mode) \ INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode) -# define open_not_cancel_2(name, flags) \ - INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags) #endif +} + +static inline int +open_not_cancel_2 (const char *name, int flags) +{ +#ifdef __NR_open + return INLINE_SYSCALL (open, 2, name, flags); +#else + return INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags); +#endif +} /* Uncancelable read. */ #define __read_nocancel(fd, buf, len) \ INLINE_SYSCALL (read, 3, fd, buf, len) /* Uncancelable write. */ -#define __write_nocancel(fd, buf, len) \ - INLINE_SYSCALL (write, 3, fd, buf, len) +static inline ssize_t +__write_nocancel(int fd, const void *buf, size_t len) +{ + return INLINE_SYSCALL (write, 3, fd, buf, len); +} /* Uncancelable openat. */ #define openat_not_cancel(fd, fname, oflag, mode) \ @@ -53,8 +68,13 @@ INLINE_SYSCALL (openat, 3, fd, fname, oflag) #define openat64_not_cancel(fd, fname, oflag, mode) \ INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode) -#define openat64_not_cancel_3(fd, fname, oflag) \ - INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE) +/*#define openat64_not_cancel_3(fd, fname, oflag) \ + INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)*/ +static inline int +openat64_not_cancel_3 (int fd, const char *fname, int oflag) +{ + return INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE); +} /* Uncancelable close. */ #define __close_nocancel(fd) \ @@ -66,17 +86,23 @@ INTERNAL_SYSCALL (close, err, 1, (fd)); }) /* Uncancelable read. */ -#define read_not_cancel(fd, buf, n) \ - __read_nocancel (fd, buf, n) +static inline ssize_t +read_not_cancel (int fd, void *buf, size_t n) +{ + return __read_nocancel (fd, buf, n); +} /* Uncancelable write. */ #define write_not_cancel(fd, buf, n) \ __write_nocancel (fd, buf, n) /* Uncancelable writev. */ -#define writev_not_cancel_no_status(fd, iov, n) \ - (void) ({ INTERNAL_SYSCALL_DECL (err); \ - INTERNAL_SYSCALL (writev, err, 3, (fd), (iov), (n)); }) +static inline void +writev_not_cancel_no_status (int fd, const struct iovec *iov, int n) +{ + INTERNAL_SYSCALL_DECL (err); + INTERNAL_SYSCALL (writev, err, 3, fd, iov, n); +} /* Uncancelable fcntl. */ #define fcntl_not_cancel(fd, cmd, val) \ diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h index 1a671e1..9c13ca8 100644 --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h @@ -273,9 +273,9 @@ LOAD_REGS_0 # define ASM_ARGS_1 ASM_ARGS_0, "r" (_a1) # define LOAD_ARGS_1(a1) \ - LOAD_ARGS_TYPES_1 (long int, a1) + LOAD_ARGS_TYPES_1 (typeof (a1), a1) # define LOAD_REGS_1 \ - LOAD_REGS_TYPES_1 (long int, a1) + LOAD_REGS_TYPES_1 (typeof (__arg1), a1) # define LOAD_ARGS_TYPES_2(t1, a1, t2, a2) \ t2 __arg2 = (t2) (a2); \ @@ -285,9 +285,9 @@ LOAD_REGS_TYPES_1(t1, a1) # define ASM_ARGS_2 ASM_ARGS_1, "r" (_a2) # define LOAD_ARGS_2(a1, a2) \ - LOAD_ARGS_TYPES_2 (long int, a1, long int, a2) + LOAD_ARGS_TYPES_2 (typeof (a1), a1, typeof (a2), a2) # define LOAD_REGS_2 \ - LOAD_REGS_TYPES_2 (long int, a1, long int, a2) + LOAD_REGS_TYPES_2 (typeof (__arg1), a1, typeof (__arg2), a2) # define LOAD_ARGS_TYPES_3(t1, a1, t2, a2, t3, a3) \ t3 __arg3 = (t3) (a3); \ @@ -297,9 +297,10 @@ LOAD_REGS_TYPES_2(t1, a1, t2, a2) # define ASM_ARGS_3 ASM_ARGS_2, "r" (_a3) # define LOAD_ARGS_3(a1, a2, a3) \ - LOAD_ARGS_TYPES_3 (long int, a1, long int, a2, long int, a3) + LOAD_ARGS_TYPES_3 (typeof (a1), a1, typeof (a2), a2, typeof (a3), a3) # define LOAD_REGS_3 \ - LOAD_REGS_TYPES_3 (long int, a1, long int, a2, long int, a3) + LOAD_REGS_TYPES_3 (typeof (__arg1), a1, typeof (__arg2), a2, \ + typeof (__arg3), a3) # define LOAD_ARGS_TYPES_4(t1, a1, t2, a2, t3, a3, t4, a4) \ t4 __arg4 = (t4) (a4); \ @@ -309,11 +310,11 @@ LOAD_REGS_TYPES_3(t1, a2, t2, a2, t3, a3) # define ASM_ARGS_4 ASM_ARGS_3, "r" (_a4) # define LOAD_ARGS_4(a1, a2, a3, a4) \ - LOAD_ARGS_TYPES_4 (long int, a1, long int, a2, long int, a3, \ - long int, a4) + LOAD_ARGS_TYPES_4 (typeof (a1), a1, typeof (a2), a2, typeof (a3), a3, \ + typeof (a4), a4) # define LOAD_REGS_4 \ - LOAD_REGS_TYPES_4 (long int, a1, long int, a2, long int, a3, \ - long int, a4) + LOAD_REGS_TYPES_4 (typeof (__arg1), a1, typeof (__arg2), a2, \ + typeof (__arg3), a3, typeof (__arg4), a4) # define LOAD_ARGS_TYPES_5(t1, a1, t2, a2, t3, a3, t4, a4, t5, a5) \ t5 __arg5 = (t5) (a5); \ @@ -323,11 +324,12 @@ LOAD_REGS_TYPES_4 (t1, a1, t2, a2, t3, a3, t4, a4) # define ASM_ARGS_5 ASM_ARGS_4, "r" (_a5) # define LOAD_ARGS_5(a1, a2, a3, a4, a5) \ - LOAD_ARGS_TYPES_5 (long int, a1, long int, a2, long int, a3, \ - long int, a4, long int, a5) + LOAD_ARGS_TYPES_5 (typeof (a1), a1, typeof (a2), a2, typeof (a3), a3, \ + typeof (a4), a4, typeof (a5), a5) # define LOAD_REGS_5 \ - LOAD_REGS_TYPES_5 (long int, a1, long int, a2, long int, a3, \ - long int, a4, long int, a5) + LOAD_REGS_TYPES_5 (typeof (__arg1), a1, typeof (__arg2), a2, \ + typeof (__arg3), a3, typeof (__arg4), a4, \ + typeof (__arg5), a5) # define LOAD_ARGS_TYPES_6(t1, a1, t2, a2, t3, a3, t4, a4, t5, a5, t6, a6) \ t6 __arg6 = (t6) (a6); \ @@ -337,11 +339,12 @@ LOAD_REGS_TYPES_5 (t1, a1, t2, a2, t3, a3, t4, a4, t5, a5) # define ASM_ARGS_6 ASM_ARGS_5, "r" (_a6) # define LOAD_ARGS_6(a1, a2, a3, a4, a5, a6) \ - LOAD_ARGS_TYPES_6 (long int, a1, long int, a2, long int, a3, \ - long int, a4, long int, a5, long int, a6) + LOAD_ARGS_TYPES_6 (typeof (a1), a1, typeof (a2), a2, typeof (a3), a3, \ + typeof (a4), a4, typeof (a5), a5, typeof (a6), a6) # define LOAD_REGS_6 \ - LOAD_REGS_TYPES_6 (long int, a1, long int, a2, long int, a3, \ - long int, a4, long int, a5, long int, a6) + LOAD_REGS_TYPES_6 (typeof (__arg1), a1, typeof (__arg2), a2, \ + typeof (__arg3), a3, typeof (__arg4), a4, \ + typeof (__arg5), a5, typeof (__arg6), a6) #endif /* __ASSEMBLER__ */