[PATCH] Fix p{readv,writev}{64} consolidation implementation

[PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

This patch fixes the p{readv,writev}{64} consolidation implementation
from commits 4e77815 and af5fdf5.  Different from pread/pwrite
implementation, preadv/pwritev implementation does not require
__ALIGNMENT_ARG because kernel syscall prototypes define
the high and low part of the off_t, if it is the case, directly
(different from pread/pwrite where the architecture ABI for passing
64-bit values must be in consideration for passsing the arguments).

It also adds some basic tests for preadv/pwritev.

Tested on x86_64, i686, and armhf.

	* misc/Makefile (tests): Add tst-preadvwritev and tst-preadvwritev64.
	* misc/tst-preadvwritev.c: New file.
	* misc/tst-preadvwritev64.c: Likewise.
	* sysdeps/unix/sysv/linux/preadv.c (preadv): Remove SYSCALL_LL{64}
	usage.
	* sysdeps/unix/sysv/linux/preadv64.c (preadv64): Likewise.
	* sysdeps/unix/sysv/linux/pwritev.c (pwritev): Likewise.
	* sysdeps/unix/sysv/linux/pwritev64.c (pwritev64): Likewise.
---
 ChangeLog                           |  11 ++++
 misc/Makefile                       |   3 +-
 misc/tst-preadvwritev.c             | 111 ++++++++++++++++++++++++++++++++++++
 misc/tst-preadvwritev64.c           |  22 +++++++
 sysdeps/unix/sysv/linux/preadv.c    |   5 +-
 sysdeps/unix/sysv/linux/preadv64.c  |   5 +-
 sysdeps/unix/sysv/linux/pwritev.c   |   5 +-
 sysdeps/unix/sysv/linux/pwritev64.c |   4 +-
 8 files changed, 154 insertions(+), 12 deletions(-)
 create mode 100644 misc/tst-preadvwritev.c
 create mode 100644 misc/tst-preadvwritev64.c

diff --git a/misc/Makefile b/misc/Makefile
index 6498adc..56e20ce 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,8 @@ gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \
+	 tst-preadvwritev tst-preadvwritev64
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/tst-preadvwritev.c b/misc/tst-preadvwritev.c
new file mode 100644
index 0000000..60c7c84
--- /dev/null
+++ b/misc/tst-preadvwritev.c
@@ -0,0 +1,111 @@
+/* Tests for preadv and pwritev.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <sys/uio.h>
+
+/* Allow testing of the 64-bit versions as well.  */
+#ifndef PREAD
+# define PREAD pread
+# define PWRITE pwrite
+#endif
+
+static void do_prepare (void);
+static int do_test (void);
+#define PREPARE(argc, argv)     do_prepare ()
+#define TEST_FUNCTION           do_test ()
+#include "../test-skeleton.c"
+
+static char *temp_filename;
+static int temp_fd;
+
+void
+do_prepare (void)
+{
+  temp_fd = create_temp_file ("tst-preadvwritev.", &temp_filename);
+  if (temp_fd == -1)
+    {
+      printf ("cannot create temporary file: %m\n");
+      exit (1);
+    }
+}
+
+#define FAIL() \
+  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
+
+int
+do_test (void)
+{
+  struct iovec iov[2];
+  ssize_t ret;
+
+  char buf1[32];
+  char buf2[64];
+
+  memset (buf1, 0x00, sizeof buf1);
+  memset (buf1, 0xca, sizeof buf1);
+
+  memset (iov, 0, sizeof iov);
+  iov[0].iov_base = buf1;
+  iov[0].iov_len = sizeof buf1;
+  iov[1].iov_base = buf2;
+  iov[1].iov_len = sizeof buf2;
+
+  ret = pwritev (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ();
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ();
+
+  ret = pwritev (temp_fd, iov, 2, sizeof buf1 + sizeof buf2);
+  if (ret == -1)
+    FAIL ();
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ();
+  
+  char buf3[32];
+  char buf4[64];
+
+  iov[0].iov_base = buf3;
+  iov[0].iov_len = sizeof buf3;
+  iov[1].iov_base = buf4;
+  iov[1].iov_len = sizeof buf4;
+
+  ret = preadv (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ();
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ();
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ();
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ();
+
+  ret = preadv (temp_fd, iov, 2, sizeof buf3 + sizeof buf4);
+  if (ret == -1)
+    FAIL ();
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ();
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ();
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ();
+
+  return 0;
+}
diff --git a/misc/tst-preadvwritev64.c b/misc/tst-preadvwritev64.c
new file mode 100644
index 0000000..ff6e134
--- /dev/null
+++ b/misc/tst-preadvwritev64.c
@@ -0,0 +1,22 @@
+/* Tests for pread64 and pwrite64.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define PREADV  preadv64
+#define PWRITEV pwritev64
+
+#include "tst-preadvwritev.c"
diff --git a/sysdeps/unix/sysv/linux/preadv.c b/sysdeps/unix/sysv/linux/preadv.c
index f6958c3..1dbaa35 100644
--- a/sysdeps/unix/sysv/linux/preadv.c
+++ b/sysdeps/unix/sysv/linux/preadv.c
@@ -29,8 +29,7 @@
 ssize_t
 preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (preadv, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (preadv, fd, vector, count, SYSCALL_LL (offset));
 }
 # else
 static ssize_t __atomic_preadv_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@ preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_preadv
   ssize_t result = SYSCALL_CANCEL (preadv, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   SYSCALL_LL (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/preadv64.c b/sysdeps/unix/sysv/linux/preadv64.c
index 18f5550..7c7910a 100644
--- a/sysdeps/unix/sysv/linux/preadv64.c
+++ b/sysdeps/unix/sysv/linux/preadv64.c
@@ -27,8 +27,7 @@
 ssize_t
 preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-  return SYSCALL_CANCEL (preadv64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+  return SYSCALL_CANCEL (preadv64, fd, vector, count, SYSCALL_LL64 (offset));
 }
 #else
 static ssize_t __atomic_preadv64_replacement (int, const struct iovec *,
@@ -38,7 +37,7 @@ preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
 #ifdef __NR_preadv64
   ssize_t result = SYSCALL_CANCEL (preadv64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   SYSCALL_LL64 (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
diff --git a/sysdeps/unix/sysv/linux/pwritev.c b/sysdeps/unix/sysv/linux/pwritev.c
index b11606c..57e579c 100644
--- a/sysdeps/unix/sysv/linux/pwritev.c
+++ b/sysdeps/unix/sysv/linux/pwritev.c
@@ -29,8 +29,7 @@
 ssize_t
 pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (pwritev, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (pwritev, fd, vector, count, SYSCALL_LL (offset));
 }
 # else
 static ssize_t __atomic_pwritev_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@ pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_pwritev
   ssize_t result = SYSCALL_CANCEL (pwritev, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   SYSCALL_LL (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/pwritev64.c b/sysdeps/unix/sysv/linux/pwritev64.c
index bed79b7..39af02f 100644
--- a/sysdeps/unix/sysv/linux/pwritev64.c
+++ b/sysdeps/unix/sysv/linux/pwritev64.c
@@ -28,7 +28,7 @@ ssize_t
 pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
   return SYSCALL_CANCEL (pwritev64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+			 SYSCALL_LL64 (offset));
 }
 #else
 static ssize_t __atomic_pwritev64_replacement (int, const struct iovec *,
@@ -38,7 +38,7 @@ pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
 #ifdef __NR_pwrite64v
   ssize_t result = SYSCALL_CANCEL (pwritev64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   SYSCALL_LL64 (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
-- 
2.7.4

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Florian Weimer]

On 06/14/2016 11:54 PM, Adhemerval Zanella wrote:
> +  char buf3[32];
> +  char buf4[64];

Please memset these buffers with a different value before calling 
preadv.  Looks good otherwise.

Thanks,
Florian

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
> This patch fixes the p{readv,writev}{64} consolidation implementation
> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
> implementation, preadv/pwritev implementation does not require
> __ALIGNMENT_ARG because kernel syscall prototypes define
> the high and low part of the off_t, if it is the case, directly
> (different from pread/pwrite where the architecture ABI for passing
> 64-bit values must be in consideration for passsing the arguments).

i had looked at that specifically but thought it ok because the old code
was using the alignment arg.  was the old code broken too ?

this is what the preadv code looked like:
-ssize_t
-__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
-{
-  assert (sizeof (offset) == 4);
-  return SYSCALL_CANCEL (preadv, fd,
-                         vector, count, __ALIGNMENT_ARG
-                         __LONG_LONG_PAIR (offset >> 31, offset));
-}

although i guess this isn't too surprising as this code was in the
generic sysdeps dir which currently doesn't have as many users as
we wish it did :).

> +#define FAIL() \
> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
> ...
> +  ret = pwritev (temp_fd, iov, 2, 0);
> +  if (ret == -1)
> +    FAIL ();

as a personal stance, never been a big fan of messages that just have a
line number.  when you get reports/logs, you end having to chase down the
exact same source tree as the reporter.

why can't we just assert() everywhere ?  we rely on that in tests already
and we wouldn't have to do any checking otherwise.
  ret = pwritev (temp_fd, iov, 2, 0);
  assert (ret == sizeof buf1 + sizeof buf2);
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Florian Weimer]

On 06/15/2016 07:37 AM, Mike Frysinger wrote:

> although i guess this isn't too surprising as this code was in the
> generic sysdeps dir which currently doesn't have as many users as
> we wish it did :).

Yeah, I think we have to rely on the new test to catch any outliers.

>> +#define FAIL() \
>> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
>> ...
>> +  ret = pwritev (temp_fd, iov, 2, 0);
>> +  if (ret == -1)
>> +    FAIL ();
>
> as a personal stance, never been a big fan of messages that just have a
> line number.  when you get reports/logs, you end having to chase down the
> exact same source tree as the reporter.

At least there is a line number.  Many existing tests do not even have that.

> why can't we just assert() everywhere ?  we rely on that in tests already
> and we wouldn't have to do any checking otherwise.
>   ret = pwritev (temp_fd, iov, 2, 0);
>   assert (ret == sizeof buf1 + sizeof buf2);

assert writes to standard error, not standard output, where it will be 
captured.  I don't know why we don't capture both.

There seems to be a policy against tests aborting on failure, too, but 
we are not very consistent about that.  In my experience, people who 
build and test glibc are more likely to shrug off tests with non-zero 
exit status than to ignore tests with aborts, although they really 
should treat them the same.

I think we should really have some sort of libtest.a, which provides 
test helpers, error-checking functions such as xmalloc, and general 
helpers for setting up special test environments.  I'm a bit worried 
about figuring out the proper dependencies, so that libtest.a is built 
before all the tests are linked.

Florian

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Pedro Alves]

On 06/14/2016 10:54 PM, Adhemerval Zanella wrote:
> +
> +  char buf1[32];
> +  char buf2[64];
> +
> +  memset (buf1, 0x00, sizeof buf1);
> +  memset (buf1, 0xca, sizeof buf1);
> +

Did you mean to memset buf2 in the second memset call?

Thanks,
Pedro Alves

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 15/06/2016 09:59, Pedro Alves wrote:
> On 06/14/2016 10:54 PM, Adhemerval Zanella wrote:
>> +
>> +  char buf1[32];
>> +  char buf2[64];
>> +
>> +  memset (buf1, 0x00, sizeof buf1);
>> +  memset (buf1, 0xca, sizeof buf1);
>> +
> 
> Did you mean to memset buf2 in the second memset call?
> 
> Thanks,
> Pedro Alves
> 

Oops yes, I will change that.

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 15/06/2016 02:37, Mike Frysinger wrote:
> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>> This patch fixes the p{readv,writev}{64} consolidation implementation
>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>> implementation, preadv/pwritev implementation does not require
>> __ALIGNMENT_ARG because kernel syscall prototypes define
>> the high and low part of the off_t, if it is the case, directly
>> (different from pread/pwrite where the architecture ABI for passing
>> 64-bit values must be in consideration for passsing the arguments).
> 
> i had looked at that specifically but thought it ok because the old code
> was using the alignment arg.  was the old code broken too ?
> 
> this is what the preadv code looked like:
> -ssize_t
> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
> -{
> -  assert (sizeof (offset) == 4);
> -  return SYSCALL_CANCEL (preadv, fd,
> -                         vector, count, __ALIGNMENT_ARG
> -                         __LONG_LONG_PAIR (offset >> 31, offset));
> -}
> 
> although i guess this isn't too surprising as this code was in the
> generic sysdeps dir which currently doesn't have as many users as
> we wish it did :).

I though it too, but before the consolidation patch only nios2 and tile 32-bits
used the generic preadv.c implementation.  And only tile 32-bits defines 
__ASSUME_ALIGNED_REGISTER_PAIRS (so __ALIGNMENT_ARG will pad the argument with 0).
However since the syscall is defined as in linux source:

fs/read_write.c:

 991 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
 992                 unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
 993 {
 994         loff_t pos = pos_from_hilo(pos_h, pos_l);
 995 
 996         return do_preadv(fd, vec, vlen, pos, 0);
 997 }


The idea is not really to align the argument to zero pass, but rather to split
the possible 64-bits argument in high and low as required (as the default 
implementation was doing [2]). On tile, it is working because the preadv.c
offset is 32-bits and thus the high word is indeed zero, but it is passing
one superfluous argument.  Also my understanding is this generic implementation
does work correctly in every architecture because __LONG_LONG_PAIR relies
on endianess. 


[1] sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c
[2] sysdeps/unix/sysv/linux/preadv.c

> 
>> +#define FAIL() \
>> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
>> ...
>> +  ret = pwritev (temp_fd, iov, 2, 0);
>> +  if (ret == -1)
>> +    FAIL ();
> 
> as a personal stance, never been a big fan of messages that just have a
> line number.  when you get reports/logs, you end having to chase down the
> exact same source tree as the reporter.
> 

Right, I will change to a more descriptive error message.

> why can't we just assert() everywhere ?  we rely on that in tests already
> and we wouldn't have to do any checking otherwise.
>   ret = pwritev (temp_fd, iov, 2, 0);
>   assert (ret == sizeof buf1 + sizeof buf2);
> -mike
> 

As Florian has stated assert writes on stderr :(

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 15/06/2016 03:50, Florian Weimer wrote:
> On 06/15/2016 07:37 AM, Mike Frysinger wrote:
> 
>> although i guess this isn't too surprising as this code was in the
>> generic sysdeps dir which currently doesn't have as many users as
>> we wish it did :).
> 
> Yeah, I think we have to rely on the new test to catch any outliers.
> 
>>> +#define FAIL() \
>>> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
>>> ...
>>> +  ret = pwritev (temp_fd, iov, 2, 0);
>>> +  if (ret == -1)
>>> +    FAIL ();
>>
>> as a personal stance, never been a big fan of messages that just have a
>> line number.  when you get reports/logs, you end having to chase down the
>> exact same source tree as the reporter.
> 
> At least there is a line number.  Many existing tests do not even have that.
> 
>> why can't we just assert() everywhere ?  we rely on that in tests already
>> and we wouldn't have to do any checking otherwise.
>>   ret = pwritev (temp_fd, iov, 2, 0);
>>   assert (ret == sizeof buf1 + sizeof buf2);
> 
> assert writes to standard error, not standard output, where it will be captured.  I don't know why we don't capture both.
> 
> There seems to be a policy against tests aborting on failure, too, but we are not very consistent about that.  In my experience, people who build and test glibc are more likely to shrug off tests with non-zero exit status than to ignore tests with aborts, although they really should treat them the same.
> 
> I think we should really have some sort of libtest.a, which provides test helpers, error-checking functions such as xmalloc, and general helpers for setting up special test environments.  I'm a bit worried about figuring out the proper dependencies, so that libtest.a is built before all the tests are linked.
> 
> Florian
> 

libtest.a is indeed good idea and there is lot of tests that reimplement the same
thing over and over.

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]

On 15 Jun 2016 08:50, Florian Weimer wrote:
> On 06/15/2016 07:37 AM, Mike Frysinger wrote:
> >> +#define FAIL() \
> >> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
> >> ...
> >> +  ret = pwritev (temp_fd, iov, 2, 0);
> >> +  if (ret == -1)
> >> +    FAIL ();
> >
> > as a personal stance, never been a big fan of messages that just have a
> > line number.  when you get reports/logs, you end having to chase down the
> > exact same source tree as the reporter.
> 
> At least there is a line number.  Many existing tests do not even have that.
> 
> > why can't we just assert() everywhere ?  we rely on that in tests already
> > and we wouldn't have to do any checking otherwise.
> >   ret = pwritev (temp_fd, iov, 2, 0);
> >   assert (ret == sizeof buf1 + sizeof buf2);
> 
> assert writes to standard error, not standard output, where it will be 
> captured.  I don't know why we don't capture both.

we keep running into this issue.  rather than try to be super vigilent
all the time, why don't we simply:

--- a/test-skeleton.c
+++ b/test-skeleton.c
@@ -343,6 +343,10 @@ main (int argc, char *argv[])
   setbuf (stdout, NULL);
 #endif
 
+  fclose (stderr);
+  dup2 (STDOUT_FILENO, STDERR_FILENO);
+  stderr = fdopen (STDERR_FILENO, "w");
+
   while ((opt = getopt_long (argc, argv, "+", options, NULL)) != -1)
     switch (opt)
       {

> There seems to be a policy against tests aborting on failure, too, but 
> we are not very consistent about that.  In my experience, people who 
> build and test glibc are more likely to shrug off tests with non-zero 
> exit status than to ignore tests with aborts, although they really 
> should treat them the same.

we've actively been adding tests with abort() usage.  i'm not sure there
is a policy against it.

i'm not sure i agree on the non-zero-vs-abort issue, but i'm not everyone.
i look at all tests that fail regardless of reason.

> I think we should really have some sort of libtest.a, which provides 
> test helpers, error-checking functions such as xmalloc, and general 
> helpers for setting up special test environments.  I'm a bit worried 
> about figuring out the proper dependencies, so that libtest.a is built 
> before all the tests are linked.

isn't this what test-skeleton.c does for us now ?  and we all agree that
all tests should be using that.

the only downside is the inclusion ordering ... a lot of tests do either:
(1) at top (allows helper funcs to be used in the test)
static int do_test (void);
#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"
(2) at bottom (does not allow access to helper funcs)
#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"

imo we should just mandate all tests use the entry point "do_test" and it
take argc/argv args (even if they're unused), and then all tests can pull
test-skeleton.c in at the top.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Chris Metcalf]

On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
> On 15/06/2016 02:37, Mike Frysinger wrote:
>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>> implementation, preadv/pwritev implementation does not require
>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>> the high and low part of the off_t, if it is the case, directly
>>> (different from pread/pwrite where the architecture ABI for passing
>>> 64-bit values must be in consideration for passsing the arguments).
>> i had looked at that specifically but thought it ok because the old code
>> was using the alignment arg.  was the old code broken too ?
>>
>> this is what the preadv code looked like:
>> -ssize_t
>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>> -{
>> -  assert (sizeof (offset) == 4);
>> -  return SYSCALL_CANCEL (preadv, fd,
>> -                         vector, count, __ALIGNMENT_ARG
>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>> -}
>>
>> although i guess this isn't too surprising as this code was in the
>> generic sysdeps dir which currently doesn't have as many users as
>> we wish it did :).
> I though it too, but before the consolidation patch only nios2 and tile 32-bits
> used the generic preadv.c implementation.  And only tile 32-bits defines
> __ASSUME_ALIGNED_REGISTER_PAIRS (so __ALIGNMENT_ARG will pad the argument with 0).
> However since the syscall is defined as in linux source:
>
> fs/read_write.c:
>
>   991 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
>   992                 unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
>   993 {
>   994         loff_t pos = pos_from_hilo(pos_h, pos_l);
>   995
>   996         return do_preadv(fd, vec, vlen, pos, 0);
>   997 }

Actually, the relevant code at fs/read_write.c:1162 is:

COMPAT_SYSCALL_DEFINE5(preadv, compat_ulong_t, fd,
         const struct compat_iovec __user *,vec,
         compat_ulong_t, vlen, u32, pos_low, u32, pos_high)
{
     loff_t pos = ((loff_t)pos_high << 32) | pos_low;

     return do_compat_preadv64(fd, vec, vlen, pos, 0);
}

but it amounts to the same thing as far the arguments are concerned.

> The idea is not really to align the argument to zero pass, but rather to split
> the possible 64-bits argument in high and low as required (as the default
> implementation was doing [2]). On tile, it is working because the preadv.c
> offset is 32-bits and thus the high word is indeed zero, but it is passing
> one superfluous argument.

No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
high offset and either fail or (for a sufficiently large file) get the wrong data.
Filed as bug 20261.

It appears to be true for both preadv and pwritev, though I only tested preadv.

> Also my understanding is this generic implementation
> does work correctly in every architecture because __LONG_LONG_PAIR relies
> on endianess.

In fact that's another bug; __LONG_LONG_PAIR is intended only to
be used if you are simulating passing a 64-bit value in 32-bit registers,
where the ABI naturally splits the 64 bits into a high and low part
according to endianness.

In this example we are calling into the kernel where it expects a pos_low
and a pos_high in a particular order.  On a big-endian machine, the
__LONG_LONG_PAIR will put the high part first and the kernel will
see the wrong offset.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 15/06/2016 15:21, Chris Metcalf wrote:
> On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
>> On 15/06/2016 02:37, Mike Frysinger wrote:
>>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>>> implementation, preadv/pwritev implementation does not require
>>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>>> the high and low part of the off_t, if it is the case, directly
>>>> (different from pread/pwrite where the architecture ABI for passing
>>>> 64-bit values must be in consideration for passsing the arguments).
>>> i had looked at that specifically but thought it ok because the old code
>>> was using the alignment arg.  was the old code broken too ?
>>>
>>> this is what the preadv code looked like:
>>> -ssize_t
>>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>>> -{
>>> -  assert (sizeof (offset) == 4);
>>> -  return SYSCALL_CANCEL (preadv, fd,
>>> -                         vector, count, __ALIGNMENT_ARG
>>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>>> -}
>>>
>>> although i guess this isn't too surprising as this code was in the
>>> generic sysdeps dir which currently doesn't have as many users as
>>> we wish it did :).
>> I though it too, but before the consolidation patch only nios2 and tile 32-bits
>> used the generic preadv.c implementation.  And only tile 32-bits defines
>> __ASSUME_ALIGNED_REGISTER_PAIRS (so __ALIGNMENT_ARG will pad the argument with 0).
>> However since the syscall is defined as in linux source:
>>
>> fs/read_write.c:
>>
>>   991 SYSCALL_DEFINE5(preadv, unsigned long, fd, const struct iovec __user *, vec,
>>   992                 unsigned long, vlen, unsigned long, pos_l, unsigned long, pos_h)
>>   993 {
>>   994         loff_t pos = pos_from_hilo(pos_h, pos_l);
>>   995
>>   996         return do_preadv(fd, vec, vlen, pos, 0);
>>   997 }
> 
> Actually, the relevant code at fs/read_write.c:1162 is:
> 
> COMPAT_SYSCALL_DEFINE5(preadv, compat_ulong_t, fd,
>         const struct compat_iovec __user *,vec,
>         compat_ulong_t, vlen, u32, pos_low, u32, pos_high)
> {
>     loff_t pos = ((loff_t)pos_high << 32) | pos_low;
> 
>     return do_compat_preadv64(fd, vec, vlen, pos, 0);
> }
> 
> but it amounts to the same thing as far the arguments are concerned.

This is for 64-bits kernel that provides the 32-bit compat syscalls.
For 32-bits kernels only it the code I referenced above.

> 
>> The idea is not really to align the argument to zero pass, but rather to split
>> the possible 64-bits argument in high and low as required (as the default
>> implementation was doing [2]). On tile, it is working because the preadv.c
>> offset is 32-bits and thus the high word is indeed zero, but it is passing
>> one superfluous argument.
> 
> No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
> are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
> high offset and either fail or (for a sufficiently large file) get the wrong data.
> Filed as bug 20261.

I was referring to *old* behaviour (pre-consolidation) implementation 
(the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:

--
  return SYSCALL_CANCEL (preadv, fd,
                         vector, count, __ALIGNMENT_ARG
                         __LONG_LONG_PAIR (offset >> 31, offset));
--

It has 2 issue:

 1. It passed one superfluous argument to preadv. On tilepro build
    I noted that is using internal_syscall6, which means it is passing
    { fd, vector, cound, 0, offset, offset>>31 }.  It is not wrong,
    since for this code off_t will be the old 32-bit value, but the
    semantic is wrong.

 2. __LONG_LONG_PAIR is not correct for big-endian.


Now for BZ#20261 I do not think it applicable since this is a fix for
consolidation done in development phase, it does not appear in any
released version.

Now, your analysis is correct for *current* code and it is contains the
__LONG_LONG_PAIR issue due the SYSCALL_LL usage.  I will change it
and post a second version.

> 
> It appears to be true for both preadv and pwritev, though I only tested preadv.
> 
>> Also my understanding is this generic implementation
>> does work correctly in every architecture because __LONG_LONG_PAIR relies
>> on endianess.
> 
> In fact that's another bug; __LONG_LONG_PAIR is intended only to
> be used if you are simulating passing a 64-bit value in 32-bit registers,
> where the ABI naturally splits the 64 bits into a high and low part
> according to endianness.
> 
> In this example we are calling into the kernel where it expects a pos_low
> and a pos_high in a particular order.  On a big-endian machine, the
> __LONG_LONG_PAIR will put the high part first and the kernel will
> see the wrong offset.
> 

[PATCH v2] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

Changes from previous version:

 - Remove SYSCALL_LL{64} usage and replace by another macro based on
   previous default implementation (LO_HI_LONG)
 - Fix some issue with testcase

--

This patch fixes the p{readv,writev}{64} consolidation implementation
from commits 4e77815 and af5fdf5.  Different from pread/pwrite
implementation, preadv/pwritev implementation does not require
__ALIGNMENT_ARG because kernel syscall prototypes define
the high and low part of the off_t, if it is the case, directly
(different from pread/pwrite where the architecture ABI for passing
64-bit values must be in consideration for passsing the arguments).

It also adds some basic tests for preadv/pwritev.

Tested on x86_64, i686, and armhf.

	* misc/Makefile (tests): Add tst-preadvwritev and tst-preadvwritev64.
	* misc/tst-preadvwritev.c: New file.
	* misc/tst-preadvwritev64.c: Likewise.
	* sysdeps/unix/sysv/linux/preadv.c (preadv): Remove SYSCALL_LL{64}
	usage.
	* sysdeps/unix/sysv/linux/preadv64.c (preadv64): Likewise.
	* sysdeps/unix/sysv/linux/pwritev.c (pwritev): Likewise.
	* sysdeps/unix/sysv/linux/pwritev64.c (pwritev64): Likewise.
	* sysdeps/unix/sysv/linux/sysdep.h (LO_HI_LONG): New macro.
---
 ChangeLog                           |  12 ++++
 misc/Makefile                       |   3 +-
 misc/tst-preadvwritev.c             | 114 ++++++++++++++++++++++++++++++++++++
 misc/tst-preadvwritev64.c           |  22 +++++++
 sysdeps/unix/sysv/linux/preadv.c    |   5 +-
 sysdeps/unix/sysv/linux/preadv64.c  |   5 +-
 sysdeps/unix/sysv/linux/pwritev.c   |   5 +-
 sysdeps/unix/sysv/linux/pwritev64.c |   7 +--
 sysdeps/unix/sysv/linux/sysdep.h    |   9 +++
 9 files changed, 168 insertions(+), 14 deletions(-)
 create mode 100644 misc/tst-preadvwritev.c
 create mode 100644 misc/tst-preadvwritev64.c

diff --git a/ChangeLog b/ChangeLog
index a4a1cea..e555444 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2016-06-14  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+	* misc/Makefile (tests): Add tst-preadvwritev and tst-preadvwritev64.
+	* misc/tst-preadvwritev.c: New file.
+	* misc/tst-preadvwritev64.c: Likewise.
+	* sysdeps/unix/sysv/linux/preadv.c (preadv): Remove SYSCALL_LL{64}
+	usage.
+	* sysdeps/unix/sysv/linux/preadv64.c (preadv64): Likewise.
+	* sysdeps/unix/sysv/linux/pwritev.c (pwritev): Likewise.
+	* sysdeps/unix/sysv/linux/pwritev64.c (pwritev64): Likewise.
+	* sysdeps/unix/sysv/linux/sysdep.h (LO_HI_LONG): New macro.
+
 2016-06-14  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #20255]
diff --git a/misc/Makefile b/misc/Makefile
index 6498adc..56e20ce 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,8 @@ gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \
+	 tst-preadvwritev tst-preadvwritev64
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/tst-preadvwritev.c b/misc/tst-preadvwritev.c
new file mode 100644
index 0000000..6836574
--- /dev/null
+++ b/misc/tst-preadvwritev.c
@@ -0,0 +1,114 @@
+/* Tests for preadv and pwritev.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <sys/uio.h>
+
+/* Allow testing of the 64-bit versions as well.  */
+#ifndef PREADV
+# define PREADV  preadv
+# define PWRITEV pwritev
+#endif
+
+static void do_prepare (void);
+static int do_test (void);
+#define PREPARE(argc, argv)     do_prepare ()
+#define TEST_FUNCTION           do_test ()
+#include "../test-skeleton.c"
+
+static char *temp_filename;
+static int temp_fd;
+
+void
+do_prepare (void)
+{
+  temp_fd = create_temp_file ("tst-PREADVwritev.", &temp_filename);
+  if (temp_fd == -1)
+    {
+      printf ("cannot create temporary file: %m\n");
+      exit (1);
+    }
+}
+
+#define FAIL(str) \
+  do { printf ("error: %s (line %d)\n", str, __LINE__); return 1; } while (0)
+
+int
+do_test (void)
+{
+  struct iovec iov[2];
+  ssize_t ret;
+
+  char buf1[32];
+  char buf2[64];
+
+  memset (buf1, 0xf0, sizeof buf1);
+  memset (buf2, 0x0f, sizeof buf2);
+
+  memset (iov, 0, sizeof iov);
+  iov[0].iov_base = buf1;
+  iov[0].iov_len = sizeof buf1;
+  iov[1].iov_base = buf2;
+  iov[1].iov_len = sizeof buf2;
+
+  ret = PWRITEV (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ("first PWRITEV returned -1");
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ("first PWRITEV returned an unexpected value");
+
+  ret = PWRITEV (temp_fd, iov, 2, sizeof buf1 + sizeof buf2);
+  if (ret == -1)
+    FAIL ("second PWRITEV returned -1");
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ("second PWRITEV returned an unexpected value");
+  
+  char buf3[32];
+  char buf4[64];
+
+  memset (buf3, 0x0f, sizeof buf3);
+  memset (buf4, 0xf0, sizeof buf4);
+
+  iov[0].iov_base = buf3;
+  iov[0].iov_len = sizeof buf3;
+  iov[1].iov_base = buf4;
+  iov[1].iov_len = sizeof buf4;
+
+  ret = PREADV (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ("first PREADV returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("first PREADV returned an unexpected value");
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ("first buffer from first PREADV different than expected");
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ("second buffer from first PREADV different than expected");
+
+  ret = PREADV (temp_fd, iov, 2, sizeof buf3 + sizeof buf4);
+  if (ret == -1)
+    FAIL ("second PREADV returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("second PREADV returned an unexpected value");
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ("first buffer from second PREADV different than expected");
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ("second buffer from second PREADV different than expected");
+
+  return 0;
+}
diff --git a/misc/tst-preadvwritev64.c b/misc/tst-preadvwritev64.c
new file mode 100644
index 0000000..ff6e134
--- /dev/null
+++ b/misc/tst-preadvwritev64.c
@@ -0,0 +1,22 @@
+/* Tests for pread64 and pwrite64.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#define PREADV  preadv64
+#define PWRITEV pwritev64
+
+#include "tst-preadvwritev.c"
diff --git a/sysdeps/unix/sysv/linux/preadv.c b/sysdeps/unix/sysv/linux/preadv.c
index f6958c3..be206e2 100644
--- a/sysdeps/unix/sysv/linux/preadv.c
+++ b/sysdeps/unix/sysv/linux/preadv.c
@@ -29,8 +29,7 @@
 ssize_t
 preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (preadv, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (preadv, fd, vector, count, LO_HI_LONG (offset));
 }
 # else
 static ssize_t __atomic_preadv_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@ preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_preadv
   ssize_t result = SYSCALL_CANCEL (preadv, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/preadv64.c b/sysdeps/unix/sysv/linux/preadv64.c
index 18f5550..64164bb 100644
--- a/sysdeps/unix/sysv/linux/preadv64.c
+++ b/sysdeps/unix/sysv/linux/preadv64.c
@@ -27,8 +27,7 @@
 ssize_t
 preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-  return SYSCALL_CANCEL (preadv64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+  return SYSCALL_CANCEL (preadv64, fd, vector, count, LO_HI_LONG (offset));
 }
 #else
 static ssize_t __atomic_preadv64_replacement (int, const struct iovec *,
@@ -38,7 +37,7 @@ preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
 #ifdef __NR_preadv64
   ssize_t result = SYSCALL_CANCEL (preadv64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
diff --git a/sysdeps/unix/sysv/linux/pwritev.c b/sysdeps/unix/sysv/linux/pwritev.c
index b11606c..19b4a10 100644
--- a/sysdeps/unix/sysv/linux/pwritev.c
+++ b/sysdeps/unix/sysv/linux/pwritev.c
@@ -29,8 +29,7 @@
 ssize_t
 pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (pwritev, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (pwritev, fd, vector, count, LO_HI_LONG (offset));
 }
 # else
 static ssize_t __atomic_pwritev_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@ pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_pwritev
   ssize_t result = SYSCALL_CANCEL (pwritev, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/pwritev64.c b/sysdeps/unix/sysv/linux/pwritev64.c
index bed79b7..c0cfe4b 100644
--- a/sysdeps/unix/sysv/linux/pwritev64.c
+++ b/sysdeps/unix/sysv/linux/pwritev64.c
@@ -27,8 +27,7 @@
 ssize_t
 pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-  return SYSCALL_CANCEL (pwritev64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+  return SYSCALL_CANCEL (pwritev64, fd, vector, count, LO_HI_LONG (offset));
 }
 #else
 static ssize_t __atomic_pwritev64_replacement (int, const struct iovec *,
@@ -36,9 +35,9 @@ static ssize_t __atomic_pwritev64_replacement (int, const struct iovec *,
 ssize_t
 pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-#ifdef __NR_pwrite64v
+#ifdef __NR_pwritev64
   ssize_t result = SYSCALL_CANCEL (pwritev64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index f2d7e05..24deb78 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -47,3 +47,12 @@
 #define SYSCALL_LL64(val) \
   __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
 #endif
+
+/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
+#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
+# define LO_HI_LONG(val) (val)
+#else
+# define LO_HI_LONG(val) \
+  (off_t) (val),                                                          \
+  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
+#endif
-- 
2.7.4

Re: [PATCH v2] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On 15 Jun 2016 18:20, Adhemerval Zanella wrote:
> --- a/sysdeps/unix/sysv/linux/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sysdep.h
> @@ -47,3 +47,12 @@
>  #define SYSCALL_LL64(val) \
>    __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
>  #endif
> +
> +/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
> +#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
> +# define LO_HI_LONG(val) (val)
> +#else
> +# define LO_HI_LONG(val) \
> +  (off_t) (val),                                                          \
> +  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
> +#endif

does this really need to be this complicated ?  we're already making an
assumption about the size for 64-bit & 64-bit/ilp32 ports, and you're
using an uint64_t cast to start with, and SYSCALL_LL64 assumes 32.
  (long) (val),
  (long) (((uint64_t) (val)) >> 32)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

[-- Attachment #1.1: Type: text/plain, Size: 1430 bytes --]



On 15/06/2016 19:38, Mike Frysinger wrote:
> On 15 Jun 2016 18:20, Adhemerval Zanella wrote:
>> --- a/sysdeps/unix/sysv/linux/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/sysdep.h
>> @@ -47,3 +47,12 @@
>>  #define SYSCALL_LL64(val) \
>>    __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
>>  #endif
>> +
>> +/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
>> +#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
>> +# define LO_HI_LONG(val) (val)
>> +#else
>> +# define LO_HI_LONG(val) \
>> +  (off_t) (val),                                                          \
>> +  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
>> +#endif
> 
> does this really need to be this complicated ?  we're already making an
> assumption about the size for 64-bit & 64-bit/ilp32 ports, and you're
> using an uint64_t cast to start with, and SYSCALL_LL64 assumes 32.
>   (long) (val),
>   (long) (((uint64_t) (val)) >> 32)
> -mike
> 

I think your suggestion should be ok, this snippet I used came from
the previous version in fact.  I will change to just

/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
# define LO_HI_LONG(val) (val)
#else
# define LO_HI_LONG(val) \
   (long) (val), \
   (long) (((uint64_t) (val)) >> 32)
#endif


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Chris Metcalf]

On 6/15/2016 4:17 PM, Adhemerval Zanella wrote:
> On 15/06/2016 15:21, Chris Metcalf wrote:
>> On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
>>> On 15/06/2016 02:37, Mike Frysinger wrote:
>>>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>>>> implementation, preadv/pwritev implementation does not require
>>>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>>>> the high and low part of the off_t, if it is the case, directly
>>>>> (different from pread/pwrite where the architecture ABI for passing
>>>>> 64-bit values must be in consideration for passsing the arguments).
>>>> i had looked at that specifically but thought it ok because the old code
>>>> was using the alignment arg.  was the old code broken too ?
>>>>
>>>> this is what the preadv code looked like:
>>>> -ssize_t
>>>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>>>> -{
>>>> -  assert (sizeof (offset) == 4);
>>>> -  return SYSCALL_CANCEL (preadv, fd,
>>>> -                         vector, count, __ALIGNMENT_ARG
>>>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>>>> -}
>>>>
>>>> although i guess this isn't too surprising as this code was in the
>>>> generic sysdeps dir which currently doesn't have as many users as
>>>> we wish it did :).
>>>>
>>> The idea is not really to align the argument to zero pass, but rather to split
>>> the possible 64-bits argument in high and low as required (as the default
>>> implementation was doing [2]). On tile, it is working because the preadv.c
>>> offset is 32-bits and thus the high word is indeed zero, but it is passing
>>> one superfluous argument.
>> No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
>> are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
>> high offset and either fail or (for a sufficiently large file) get the wrong data.
>> Filed as bug 20261.
> I was referring to *old* behaviour (pre-consolidation) implementation
> (the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:
>
> --
>    return SYSCALL_CANCEL (preadv, fd,
>                           vector, count, __ALIGNMENT_ARG
>                           __LONG_LONG_PAIR (offset >> 31, offset));
> --
>
> It has 2 issue:
>
>   1. It passed one superfluous argument to preadv. On tilepro build
>      I noted that is using internal_syscall6, which means it is passing
>      { fd, vector, cound, 0, offset, offset>>31 }.  It is not wrong,
>      since for this code off_t will be the old 32-bit value, but the
>      semantic is wrong.

No, it's definitely wrong :-)

We pass "offset" as the high part, and "0" as the low part. Accordingly, what
the kernel sees is a request for "offset << 32" rather than "offset".  The bug
report I filed contains a repro that does fail on tilegx32.

>   2. __LONG_LONG_PAIR is not correct for big-endian.

Yes, this needs to be fixed on the kernel side for consistency. Many other
syscalls pass their arguments with __LONG_LONG_PAIR.  Handling it this way
in the kernel seems pretty standard at this point, e.g. regs_to_64 in
arch/arm64/include/asm/assembler.h, or merge_64 in
arch/mips/kernel/linux32.c.  Even the fixed-endian platforms do it
consistently, e.g. "hi, lo" on parisc/s390/sparc and "lo, hi" on metag/x86.

It's possible little-endian compat powerpc shares this bug since it seems to
take fixed "hi, lo" arguments, e.g. in compat_sys_readahead().  I'll
forward this email on to the powerpc kernel maintainers to make sure
they have a chance to think about it.

See https://lkml.kernel.org/g/1466019219-10462-1-git-send-email-cmetcalf@mellanox.com

> Now for BZ#20261 I do not think it applicable since this is a fix for
> consolidation done in development phase, it does not appear in any
> released version.

It breaks on all versions, as far as I can tell.  I tested it on our RHEL 6 platform
(based on backported glibc 2.12) and it fails there.

> Now, your analysis is correct for *current* code and it is contains the
> __LONG_LONG_PAIR issue due the SYSCALL_LL usage.  I will change it
> and post a second version.

I think you should revert that part of the change and stick with __LONG_LONG_PAIR.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 16/06/2016 12:25, Chris Metcalf wrote:
> On 6/15/2016 4:17 PM, Adhemerval Zanella wrote:
>> On 15/06/2016 15:21, Chris Metcalf wrote:
>>> On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
>>>> On 15/06/2016 02:37, Mike Frysinger wrote:
>>>>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>>>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>>>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>>>>> implementation, preadv/pwritev implementation does not require
>>>>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>>>>> the high and low part of the off_t, if it is the case, directly
>>>>>> (different from pread/pwrite where the architecture ABI for passing
>>>>>> 64-bit values must be in consideration for passsing the arguments).
>>>>> i had looked at that specifically but thought it ok because the old code
>>>>> was using the alignment arg.  was the old code broken too ?
>>>>>
>>>>> this is what the preadv code looked like:
>>>>> -ssize_t
>>>>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>>>>> -{
>>>>> -  assert (sizeof (offset) == 4);
>>>>> -  return SYSCALL_CANCEL (preadv, fd,
>>>>> -                         vector, count, __ALIGNMENT_ARG
>>>>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>>>>> -}
>>>>>
>>>>> although i guess this isn't too surprising as this code was in the
>>>>> generic sysdeps dir which currently doesn't have as many users as
>>>>> we wish it did :).
>>>>>
>>>> The idea is not really to align the argument to zero pass, but rather to split
>>>> the possible 64-bits argument in high and low as required (as the default
>>>> implementation was doing [2]). On tile, it is working because the preadv.c
>>>> offset is 32-bits and thus the high word is indeed zero, but it is passing
>>>> one superfluous argument.
>>> No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
>>> are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
>>> high offset and either fail or (for a sufficiently large file) get the wrong data.
>>> Filed as bug 20261.
>> I was referring to *old* behaviour (pre-consolidation) implementation
>> (the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:
>>
>> -- 
>>    return SYSCALL_CANCEL (preadv, fd,
>>                           vector, count, __ALIGNMENT_ARG
>>                           __LONG_LONG_PAIR (offset >> 31, offset));
>> -- 
>>
>> It has 2 issue:
>>
>>   1. It passed one superfluous argument to preadv. On tilepro build
>>      I noted that is using internal_syscall6, which means it is passing
>>      { fd, vector, cound, 0, offset, offset>>31 }.  It is not wrong,
>>      since for this code off_t will be the old 32-bit value, but the
>>      semantic is wrong.
> 
> No, it's definitely wrong :-)
> 
> We pass "offset" as the high part, and "0" as the low part. Accordingly, what
> the kernel sees is a request for "offset << 32" rather than "offset".  The bug
> report I filed contains a repro that does fail on tilegx32.

I am not following because I am referring to the old code that was *suppose*
to work on tile (either on 32-bits kernel or 64-bits kernel with compat
enabled). I rechecked the compilation against GLIBC 2.23 (which does not 
contain this consolidation) and I am seeing tilepro passing:

"R00" (fd), "R01" (vector), "R02" (count), "R03" (0), "R04" (offset), "R05" (offset >> 31)

Which indeed seems wrong with the issue you pointed out.  So how was this
suppose to work on previous GLIBC?

The new consolidation implementation indeed contains a bug and that's what
my v2 [1] is trying to fix.  With the v2 I am not seeing:

"R00" (fd), "R01" (vector), "R02" (count), "R03" ((long) (offset)), "R04" ((long) (((uint64_t) (offset)) >> 32))

[1] https://sourceware.org/ml/libc-alpha/2016-06/msg00607.html

> 
>>   2. __LONG_LONG_PAIR is not correct for big-endian.
> 
> Yes, this needs to be fixed on the kernel side for consistency. Many other
> syscalls pass their arguments with __LONG_LONG_PAIR.  Handling it this way
> in the kernel seems pretty standard at this point, e.g. regs_to_64 in
> arch/arm64/include/asm/assembler.h, or merge_64 in
> arch/mips/kernel/linux32.c.  Even the fixed-endian platforms do it
> consistently, e.g. "hi, lo" on parisc/s390/sparc and "lo, hi" on metag/x86.
> 
> It's possible little-endian compat powerpc shares this bug since it seems to
> take fixed "hi, lo" arguments, e.g. in compat_sys_readahead().  I'll
> forward this email on to the powerpc kernel maintainers to make sure
> they have a chance to think about it.
> 
> See https://lkml.kernel.org/g/1466019219-10462-1-git-send-email-cmetcalf@mellanox.com

I *really* do not think this is correct approach mainly because it break an
already defined kernel ABI and old BE preadv call on GLIBC was not done
by using __LONG_LONG_PAIR.  Old GLIBC will just fail in this new kernel
interface.  Unfortunately this is something we will need to carry on IMHO.

That's why I just removed the wrong assumption in old
sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64} about __LONG_LONG_PAIR.
 

>> Now for BZ#20261 I do not think it applicable since this is a fix for
>> consolidation done in development phase, it does not appear in any
>> released version.
> 
> It breaks on all versions, as far as I can tell.  I tested it on our RHEL 6 platform
> (based on backported glibc 2.12) and it fails there.

What exactly breaks in all version? What I am referring is that not the 
bug is 'invalid', but rather it does not apply to a *user visible* one
because the consolidation code is only in 2.24 master branch (it is not
in any *released* version).

Now, if it fails on older releases it is not due this change, but rather
due old sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64}.  Now, if
it is the case it is indeed a user visible bug.

> 
>> Now, your analysis is correct for *current* code and it is contains the
>> __LONG_LONG_PAIR issue due the SYSCALL_LL usage.  I will change it
>> and post a second version.
> 
> I think you should revert that part of the change and stick with __LONG_LONG_PAIR.
> 

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Andreas Schwab]

Chris Metcalf <cmetcalf@mellanox.com> writes:

>>   2. __LONG_LONG_PAIR is not correct for big-endian.
>
> Yes, this needs to be fixed on the kernel side for consistency. Many other
> syscalls pass their arguments with __LONG_LONG_PAIR.

__LONG_LONG_PAIR is only for the cases where the kernel uses a single
loff_t argument.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Chris Metcalf]

(+Chung-Lin Tang for possible nios2 bug.)

On 6/16/2016 11:49 AM, Adhemerval Zanella wrote:
> On 16/06/2016 12:25, Chris Metcalf wrote:
>> On 6/15/2016 4:17 PM, Adhemerval Zanella wrote:
>>> On 15/06/2016 15:21, Chris Metcalf wrote:
>>>> On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
>>>>> On 15/06/2016 02:37, Mike Frysinger wrote:
>>>>>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>>>>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>>>>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>>>>>> implementation, preadv/pwritev implementation does not require
>>>>>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>>>>>> the high and low part of the off_t, if it is the case, directly
>>>>>>> (different from pread/pwrite where the architecture ABI for passing
>>>>>>> 64-bit values must be in consideration for passsing the arguments).
>>>>>> i had looked at that specifically but thought it ok because the old code
>>>>>> was using the alignment arg.  was the old code broken too ?
>>>>>>
>>>>>> this is what the preadv code looked like:
>>>>>> -ssize_t
>>>>>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>>>>>> -{
>>>>>> -  assert (sizeof (offset) == 4);
>>>>>> -  return SYSCALL_CANCEL (preadv, fd,
>>>>>> -                         vector, count, __ALIGNMENT_ARG
>>>>>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>>>>>> -}
>>>>>>
>>>>>> although i guess this isn't too surprising as this code was in the
>>>>>> generic sysdeps dir which currently doesn't have as many users as
>>>>>> we wish it did :).
>>>>>>
>>>>> The idea is not really to align the argument to zero pass, but rather to split
>>>>> the possible 64-bits argument in high and low as required (as the default
>>>>> implementation was doing [2]). On tile, it is working because the preadv.c
>>>>> offset is 32-bits and thus the high word is indeed zero, but it is passing
>>>>> one superfluous argument.
>>>> No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
>>>> are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
>>>> high offset and either fail or (for a sufficiently large file) get the wrong data.
>>>> Filed as bug 20261.
>>> I was referring to *old* behaviour (pre-consolidation) implementation
>>> (the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:
>>>
>>> -- 
>>>     return SYSCALL_CANCEL (preadv, fd,
>>>                            vector, count, __ALIGNMENT_ARG
>>>                            __LONG_LONG_PAIR (offset >> 31, offset));
>>> -- 
>>>
>>> It has 2 issue:
>>>
>>>    1. It passed one superfluous argument to preadv. On tilepro build
>>>       I noted that is using internal_syscall6, which means it is passing
>>>       { fd, vector, cound, 0, offset, offset>>31 }.  It is not wrong,
>>>       since for this code off_t will be the old 32-bit value, but the
>>>       semantic is wrong.
>> No, it's definitely wrong :-)
>>
>> We pass "offset" as the high part, and "0" as the low part. Accordingly, what
>> the kernel sees is a request for "offset << 32" rather than "offset".  The bug
>> report I filed contains a repro that does fail on tilegx32.
> I am not following because I am referring to the old code that was *suppose*
> to work on tile (either on 32-bits kernel or 64-bits kernel with compat
> enabled). I rechecked the compilation against GLIBC 2.23 (which does not
> contain this consolidation) and I am seeing tilepro passing:
>
> "R00" (fd), "R01" (vector), "R02" (count), "R03" (0), "R04" (offset), "R05" (offset >> 31)

This is wrong, and this was indeed a bug prior to your consolidation patch.
This is a generic/wordsize-32 specific bug which apparently was not caught
during testing of tilepro or tilegx32.  In addition to the erroneous __ALIGNMENT_ARG,
we should be using LO_HI_LONG() here rather than __LONG_LONG_PAIR.
I suspect this is also a bug in the nios2 glibc.

> Which indeed seems wrong with the issue you pointed out.  So how was this
> suppose to work on previous GLIBC?

It doesn't.  That's why I filed the bug.

> The new consolidation implementation indeed contains a bug and that's what
> my v2 [1] is trying to fix.  With the v2 I am not seeing:
>
> "R00" (fd), "R01" (vector), "R02" (count), "R03" ((long) (offset)), "R04" ((long) (((uint64_t) (offset)) >> 32))
>
> [1]https://sourceware.org/ml/libc-alpha/2016-06/msg00607.html

When you say "not seeing", I'm assuming you mean "now seeing".

In any case, "vector, count, lo, hi" is what we should see here.

>>>    2. __LONG_LONG_PAIR is not correct for big-endian.
>> Yes, this needs to be fixed on the kernel side for consistency. Many other
>> syscalls pass their arguments with __LONG_LONG_PAIR.  Handling it this way
>> in the kernel seems pretty standard at this point, e.g. regs_to_64 in
>> arch/arm64/include/asm/assembler.h, or merge_64 in
>> arch/mips/kernel/linux32.c.  Even the fixed-endian platforms do it
>> consistently, e.g. "hi, lo" on parisc/s390/sparc and "lo, hi" on metag/x86.
>>
>> It's possible little-endian compat powerpc shares this bug since it seems to
>> take fixed "hi, lo" arguments, e.g. in compat_sys_readahead().  I'll
>> forward this email on to the powerpc kernel maintainers to make sure
>> they have a chance to think about it.
>>
>> Seehttps://lkml.kernel.org/g/1466019219-10462-1-git-send-email-cmetcalf@mellanox.com
> I *really* do not think this is correct approach mainly because it break an
> already defined kernel ABI and old BE preadv call on GLIBC was not done
> by using __LONG_LONG_PAIR.  Old GLIBC will just fail in this new kernel
> interface.  Unfortunately this is something we will need to carry on IMHO.
>
> That's why I just removed the wrong assumption in old
> sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64} about __LONG_LONG_PAIR.

Yes, there are two issues here that I am conflating - sorry about that.

The preadv/pwritev stuff should be using LO_HI_LONG.  I think your
v2 patch is correct.  No kernel change is required here.

When I went and reviewed this stuff for tile, it led me to the tile kernel ABI
code that was not properly handling __LONG_LONG_PAIR in the kernel
for APIs that expect 64-bit values like truncate/ftruncate, pread64/pwrite64,
sync_file_range, and fallocate.

I think our installed base of "tilepro/tilegx32 big-endian" is so small as to
be effectively zero.  We had one or two customers interested in big-endian 64-bit,
and a couple of customers interested in tilegx compat 32-bit, but I don't think we
ever had customers interested in the cross product of those two features.

Generally speaking I would otherwise agree and say we should provide
tile-specific variants of all those broken syscalls in glibc to support the existing ABI.

>>> Now for BZ#20261 I do not think it applicable since this is a fix for
>>> consolidation done in development phase, it does not appear in any
>>> released version.
>> It breaks on all versions, as far as I can tell.  I tested it on our RHEL 6 platform
>> (based on backported glibc 2.12) and it fails there.
> What exactly breaks in all version? What I am referring is that not the
> bug is 'invalid', but rather it does not apply to a *user visible* one
> because the consolidation code is only in 2.24 master branch (it is not
> in any *released* version).
>
> Now, if it fails on older releases it is not due this change, but rather
> due old sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64}.  Now, if
> it is the case it is indeed a user visible bug.

Yes, that's exactly the case for preadv/pwritev, due to the bogus _ALIGNMENT_ARG;
it breaks on all 32-bit tile platforms with all public releases.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Adhemerval Zanella]

On 20/06/2016 16:26, Chris Metcalf wrote:
> (+Chung-Lin Tang for possible nios2 bug.)
> 
> On 6/16/2016 11:49 AM, Adhemerval Zanella wrote:
>> On 16/06/2016 12:25, Chris Metcalf wrote:
>>> On 6/15/2016 4:17 PM, Adhemerval Zanella wrote:
>>>> On 15/06/2016 15:21, Chris Metcalf wrote:
>>>>> On 6/15/2016 9:45 AM, Adhemerval Zanella wrote:
>>>>>> On 15/06/2016 02:37, Mike Frysinger wrote:
>>>>>>> On 14 Jun 2016 18:54, Adhemerval Zanella wrote:
>>>>>>>> This patch fixes the p{readv,writev}{64} consolidation implementation
>>>>>>>> from commits 4e77815 and af5fdf5.  Different from pread/pwrite
>>>>>>>> implementation, preadv/pwritev implementation does not require
>>>>>>>> __ALIGNMENT_ARG because kernel syscall prototypes define
>>>>>>>> the high and low part of the off_t, if it is the case, directly
>>>>>>>> (different from pread/pwrite where the architecture ABI for passing
>>>>>>>> 64-bit values must be in consideration for passsing the arguments).
>>>>>>> i had looked at that specifically but thought it ok because the old code
>>>>>>> was using the alignment arg.  was the old code broken too ?
>>>>>>>
>>>>>>> this is what the preadv code looked like:
>>>>>>> -ssize_t
>>>>>>> -__libc_preadv (int fd, const struct iovec *vector, int count, off_t offset)
>>>>>>> -{
>>>>>>> -  assert (sizeof (offset) == 4);
>>>>>>> -  return SYSCALL_CANCEL (preadv, fd,
>>>>>>> -                         vector, count, __ALIGNMENT_ARG
>>>>>>> -                         __LONG_LONG_PAIR (offset >> 31, offset));
>>>>>>> -}
>>>>>>>
>>>>>>> although i guess this isn't too surprising as this code was in the
>>>>>>> generic sysdeps dir which currently doesn't have as many users as
>>>>>>> we wish it did :).
>>>>>>>
>>>>>> The idea is not really to align the argument to zero pass, but rather to split
>>>>>> the possible 64-bits argument in high and low as required (as the default
>>>>>> implementation was doing [2]). On tile, it is working because the preadv.c
>>>>>> offset is 32-bits and thus the high word is indeed zero, but it is passing
>>>>>> one superfluous argument.
>>>>> No, what happens is that instead of passing r3=pos_low=lo, r4=pos_high=0, we
>>>>> are passing r3=pos_low=0, r4=pos_high=lo, r5=0.  This means that we get a crazy
>>>>> high offset and either fail or (for a sufficiently large file) get the wrong data.
>>>>> Filed as bug 20261.
>>>> I was referring to *old* behaviour (pre-consolidation) implementation
>>>> (the sysdeps/unix/sysv/linux/generic/wordsize-32/preadv.c), which did:
>>>>
>>>> -- 
>>>>     return SYSCALL_CANCEL (preadv, fd,
>>>>                            vector, count, __ALIGNMENT_ARG
>>>>                            __LONG_LONG_PAIR (offset >> 31, offset));
>>>> -- 
>>>>
>>>> It has 2 issue:
>>>>
>>>>    1. It passed one superfluous argument to preadv. On tilepro build
>>>>       I noted that is using internal_syscall6, which means it is passing
>>>>       { fd, vector, cound, 0, offset, offset>>31 }.  It is not wrong,
>>>>       since for this code off_t will be the old 32-bit value, but the
>>>>       semantic is wrong.
>>> No, it's definitely wrong :-)
>>>
>>> We pass "offset" as the high part, and "0" as the low part. Accordingly, what
>>> the kernel sees is a request for "offset << 32" rather than "offset".  The bug
>>> report I filed contains a repro that does fail on tilegx32.
>> I am not following because I am referring to the old code that was *suppose*
>> to work on tile (either on 32-bits kernel or 64-bits kernel with compat
>> enabled). I rechecked the compilation against GLIBC 2.23 (which does not
>> contain this consolidation) and I am seeing tilepro passing:
>>
>> "R00" (fd), "R01" (vector), "R02" (count), "R03" (0), "R04" (offset), "R05" (offset >> 31)
> 
> This is wrong, and this was indeed a bug prior to your consolidation patch.
> This is a generic/wordsize-32 specific bug which apparently was not caught
> during testing of tilepro or tilegx32.  In addition to the erroneous __ALIGNMENT_ARG,
> we should be using LO_HI_LONG() here rather than __LONG_LONG_PAIR.
> I suspect this is also a bug in the nios2 glibc.

Ah right then.

> 
>> Which indeed seems wrong with the issue you pointed out.  So how was this
>> suppose to work on previous GLIBC?
> 
> It doesn't.  That's why I filed the bug.

Ack.

> 
>> The new consolidation implementation indeed contains a bug and that's what
>> my v2 [1] is trying to fix.  With the v2 I am not seeing:
>>
>> "R00" (fd), "R01" (vector), "R02" (count), "R03" ((long) (offset)), "R04" ((long) (((uint64_t) (offset)) >> 32))
>>
>> [1]https://sourceware.org/ml/libc-alpha/2016-06/msg00607.html
> 
> When you say "not seeing", I'm assuming you mean "now seeing".
> 
> In any case, "vector, count, lo, hi" is what we should see here.

Yes, s/not/now.  Right, that is my understanding as well.

> 
>>>>    2. __LONG_LONG_PAIR is not correct for big-endian.
>>> Yes, this needs to be fixed on the kernel side for consistency. Many other
>>> syscalls pass their arguments with __LONG_LONG_PAIR.  Handling it this way
>>> in the kernel seems pretty standard at this point, e.g. regs_to_64 in
>>> arch/arm64/include/asm/assembler.h, or merge_64 in
>>> arch/mips/kernel/linux32.c.  Even the fixed-endian platforms do it
>>> consistently, e.g. "hi, lo" on parisc/s390/sparc and "lo, hi" on metag/x86.
>>>
>>> It's possible little-endian compat powerpc shares this bug since it seems to
>>> take fixed "hi, lo" arguments, e.g. in compat_sys_readahead().  I'll
>>> forward this email on to the powerpc kernel maintainers to make sure
>>> they have a chance to think about it.
>>>
>>> Seehttps://lkml.kernel.org/g/1466019219-10462-1-git-send-email-cmetcalf@mellanox.com
>> I *really* do not think this is correct approach mainly because it break an
>> already defined kernel ABI and old BE preadv call on GLIBC was not done
>> by using __LONG_LONG_PAIR.  Old GLIBC will just fail in this new kernel
>> interface.  Unfortunately this is something we will need to carry on IMHO.
>>
>> That's why I just removed the wrong assumption in old
>> sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64} about __LONG_LONG_PAIR.
> 
> Yes, there are two issues here that I am conflating - sorry about that.
> 
> The preadv/pwritev stuff should be using LO_HI_LONG.  I think your
> v2 patch is correct.  No kernel change is required here.

Nice, I was with the wrong assumption you had concerns regarding preadv/writev
patch.

> 
> When I went and reviewed this stuff for tile, it led me to the tile kernel ABI
> code that was not properly handling __LONG_LONG_PAIR in the kernel
> for APIs that expect 64-bit values like truncate/ftruncate, pread64/pwrite64,
> sync_file_range, and fallocate.
> 
> I think our installed base of "tilepro/tilegx32 big-endian" is so small as to
> be effectively zero.  We had one or two customers interested in big-endian 64-bit,
> and a couple of customers interested in tilegx compat 32-bit, but I don't think we
> ever had customers interested in the cross product of those two features.
> 
> Generally speaking I would otherwise agree and say we should provide
> tile-specific variants of all those broken syscalls in glibc to support the existing ABI.

Right, I was assuming a more conservative approach regarding kernel/libc
API.  But I see your point regarding the base installation.

> 
>>>> Now for BZ#20261 I do not think it applicable since this is a fix for
>>>> consolidation done in development phase, it does not appear in any
>>>> released version.
>>> It breaks on all versions, as far as I can tell.  I tested it on our RHEL 6 platform
>>> (based on backported glibc 2.12) and it fails there.
>> What exactly breaks in all version? What I am referring is that not the
>> bug is 'invalid', but rather it does not apply to a *user visible* one
>> because the consolidation code is only in 2.24 master branch (it is not
>> in any *released* version).
>>
>> Now, if it fails on older releases it is not due this change, but rather
>> due old sysdeps/unix/sysv/linux/generic/wordsize-32/preadv{64}.  Now, if
>> it is the case it is indeed a user visible bug.
> 
> Yes, that's exactly the case for preadv/pwritev, due to the bogus _ALIGNMENT_ARG;
> it breaks on all 32-bit tile platforms with all public releases.
> 

Right, I think we now aligned about pread/pwritev and I will commit it soon.  Thanks
for the reply! 

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Florian Weimer]

On 06/15/2016 05:41 PM, Mike Frysinger wrote:

>> I think we should really have some sort of libtest.a, which provides 
>> test helpers, error-checking functions such as xmalloc, and general 
>> helpers for setting up special test environments.  I'm a bit worried 
>> about figuring out the proper dependencies, so that libtest.a is built 
>> before all the tests are linked.
> 
> isn't this what test-skeleton.c does for us now ?  and we all agree that
> all tests should be using that.

There are limits to what we can put into test-skeleton.c due to symbol
conflicts and dependencies of the test harness which are incompatible
with some things which we want to test.

With libtest.a, we can still have clean compilation environments for
tests, and the linker will sort out things for us.  (We can even have
ELF constructors which get pulled into the link as needed.)

> imo we should just mandate all tests use the entry point "do_test" and it
> take argc/argv args (even if they're unused), and then all tests can pull
> test-skeleton.c in at the top.

Yes, seems a reasonable improvement, and mostly unrelated.

Thanks,
Florian

Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2222 bytes --]

On 22 Jun 2016 11:57, Florian Weimer wrote:
> On 06/15/2016 05:41 PM, Mike Frysinger wrote:
> >> I think we should really have some sort of libtest.a, which provides 
> >> test helpers, error-checking functions such as xmalloc, and general 
> >> helpers for setting up special test environments.  I'm a bit worried 
> >> about figuring out the proper dependencies, so that libtest.a is built 
> >> before all the tests are linked.
> > 
> > isn't this what test-skeleton.c does for us now ?  and we all agree that
> > all tests should be using that.
> 
> There are limits to what we can put into test-skeleton.c due to symbol
> conflicts and dependencies of the test harness which are incompatible
> with some things which we want to test.

symbol-wise, i don't think there's a realistic problem.  we've rarely
(ever?) run into this problem, and it's trivial to sort out by using
a namespace prefix like "tst_".  which we probably want to do anyways
to keep the API more readable.

dependency wise (e.g. dl or pthread usage), i'd wait until the need
actually arises before i'd start worrying about it.

> With libtest.a, we can still have clean compilation environments for
> tests, and the linker will sort out things for us.  (We can even have
> ELF constructors which get pulled into the link as needed.)

i don't think i'd agree with that.  if our test API used tst_xxx and
random tests started interposing their own variant, that's just ugly
and a pita to digest to the point where it shouldn't really be allowed.

> > imo we should just mandate all tests use the entry point "do_test" and it
> > take argc/argv args (even if they're unused), and then all tests can pull
> > test-skeleton.c in at the top.
> 
> Yes, seems a reasonable improvement, and mostly unrelated.

they are related when you look at the implications of the two paths.
if we have a single test-skeleton.c, having a sep header for all the
prototypes is kind of pointless and just busy work.  but if there's a
lib of test files, then having a sep header is pretty much required.
if there isn't a sep header, then the include point of the skeleton
must be at the top since it's acting as the header.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

libtest.a (was: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation)

[Florian Weimer]

On 06/22/2016 01:05 PM, Mike Frysinger wrote:
> On 22 Jun 2016 11:57, Florian Weimer wrote:
>> On 06/15/2016 05:41 PM, Mike Frysinger wrote:
>>>> I think we should really have some sort of libtest.a, which provides
>>>> test helpers, error-checking functions such as xmalloc, and general
>>>> helpers for setting up special test environments.  I'm a bit worried
>>>> about figuring out the proper dependencies, so that libtest.a is built
>>>> before all the tests are linked.
>>>
>>> isn't this what test-skeleton.c does for us now ?  and we all agree that
>>> all tests should be using that.
>>
>> There are limits to what we can put into test-skeleton.c due to symbol
>> conflicts and dependencies of the test harness which are incompatible
>> with some things which we want to test.
>
> symbol-wise, i don't think there's a realistic problem.  we've rarely
> (ever?) run into this problem, and it's trivial to sort out by using
> a namespace prefix like "tst_".  which we probably want to do anyways
> to keep the API more readable.

What I mean is that the test harness pulls in stuff that cause things to 
interfere with what we want to test.  This could be magic symbols for 
stdio/libio compatibility, pthread symbols, or just calling mallopt (as 
discussed before).

If the test helpers are not in just one monolithic .c file, it helps 
with achieving that.

> dependency wise (e.g. dl or pthread usage), i'd wait until the need
> actually arises before i'd start worrying about it.

The need for pthread error-checking wrappers is already there.  Our 
current policy is to check error returns from *all* pthread functions, 
and getting a nice error from the usually looks like this:

   int ret = ...;
   if (ret != 0)
     {
       errno = ret;
       printf ("error: ...: %m\n");
       _exit (1);
     }

with some special exceptions such us pthread_barrier_wait.  Many tests 
are littered with that.

>>> imo we should just mandate all tests use the entry point "do_test" and it
>>> take argc/argv args (even if they're unused), and then all tests can pull
>>> test-skeleton.c in at the top.
>>
>> Yes, seems a reasonable improvement, and mostly unrelated.
>
> they are related when you look at the implications of the two paths.
> if we have a single test-skeleton.c, having a sep header for all the
> prototypes is kind of pointless and just busy work.  but if there's a
> lib of test files, then having a sep header is pretty much required.
> if there isn't a sep header, then the include point of the skeleton
> must be at the top since it's acting as the header.

In both cases, we want the include at the top, so the change your 
proposed goes in the right direction, whether we want a libtest.a or not.

Thanks,
Florian

Re: libtest.a (was: Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation)

[Joseph Myers]

On Thu, 23 Jun 2016, Florian Weimer wrote:

> What I mean is that the test harness pulls in stuff that cause things to
> interfere with what we want to test.  This could be magic symbols for
> stdio/libio compatibility, pthread symbols, or just calling mallopt (as
> discussed before).
> 
> If the test helpers are not in just one monolithic .c file, it helps with
> achieving that.

Also, test-skeleton.c doesn't work well with tests that wish to undefine 
_LIBC and _GNU_SOURCE in order to test non-GNU feature test macros / APIs 
/ strict standards conformance options, since it uses GNU APIs itself.  
If we change things so that most of the functionality is in a separate 
library or object (built with the usual _GNU_SOURCE), and test-skeleton 
itself is very minimal, then more tests can be made to use it.  (Of 
course, doing this means doing something about the various macros tests 
can define to change how test-skeleton behaves.)

-- 
Joseph S. Myers
joseph@codesourcery.com