This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] x86-64: Add p{read,write}[v]64 to syscalls.list [BZ #20348]



On 12/07/2016 16:03, H.J. Lu wrote:
> On Tue, Jul 12, 2016 at 7:04 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> 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 <errno.h>
 #include <unistd.h>
 #include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/uio.h>
+#include <fcntl.h>
 
 /* 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__ */


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]