[v2] Fix LO_HI_LONG definition

Message ID 1467834756-21898-1-git-send-email-adhemerval.zanella@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Netto July 6, 2016, 7:52 p.m. UTC
  The p{read,write}v{64} consolidation patch [1] added a wrong guard
for LO_HI_LONG definition.  It currently uses both
'__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set
the value to be passed in one argument, otherwise it will be split
in two.  Since kernel commit 601cc11d054 the non-compat 
preadv/pwritev in same order, so the LO_HI_LONG is the same regardless 
off/off64_t size or wordsize.

The tests were passing on 64-bit because files were small enough so 
the high part of offset is 0 regardless.  This patch also changes the
tst-preadvwritev64 test to check reads and writes on a sparse file
larger than 4G.

Checked on x86_64, i686, x32, aarch64, armhf, and s390.

	* sysdeps/unix/sysv/linux/sysdep.h
	[__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): Remove
	guards.
	* misc/tst-preadvwritev-common.c: New file.
	* misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c.
	* misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and add
	a check for files larger than 2GB.

[1] 4751bbe2ad4d1bfa05774e29376d553ecfe563b0
---
 ChangeLog                        |   8 +++
 misc/tst-preadvwritev-common.c   | 112 +++++++++++++++++++++++++++++++++++++++
 misc/tst-preadvwritev.c          |  93 +-------------------------------
 misc/tst-preadvwritev64.c        |  26 +++++++--
 sysdeps/unix/sysv/linux/sysdep.h |  10 ++--
 5 files changed, 148 insertions(+), 101 deletions(-)
 create mode 100644 misc/tst-preadvwritev-common.c
  

Comments

Florian Weimer July 7, 2016, 6:56 a.m. UTC | #1
On 07/06/2016 09:52 PM, Adhemerval Zanella wrote:
> +  /* Create a sparse file larger than 4GB to check if offset is handled
> +     correctly in p{write,read}v64. */
> +  off_t base_offset = UINT32_MAX;
> +  off_t fsize = base_offset + 2048;
> +  if (ftruncate (temp_fd, fsize) != 0)
> +    {
> +      printf ("error: ftruncate (%jd) failed: %m", (intmax_t) fsize);
> +      return 1;
> +    }

I don't think this is is necessary.  You should be able to write beyond 
the current EOF and get a sparse file directly.

I'm not sure if the current tests catch calling convention errors 
because the read and write offsets could be mangled in the same way. 
Maybe you can add a test which performs a pwrite with a large offset and 
check that the file size reported by fstat is as expected?

Thanks,
Florian
  
H.J. Lu July 11, 2016, 9:09 p.m. UTC | #2
On Wed, Jul 6, 2016 at 12:52 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> The p{read,write}v{64} consolidation patch [1] added a wrong guard
> for LO_HI_LONG definition.  It currently uses both
> '__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set
> the value to be passed in one argument, otherwise it will be split
> in two.  Since kernel commit 601cc11d054 the non-compat
> preadv/pwritev in same order, so the LO_HI_LONG is the same regardless
> off/off64_t size or wordsize.
>
> The tests were passing on 64-bit because files were small enough so
> the high part of offset is 0 regardless.  This patch also changes the
> tst-preadvwritev64 test to check reads and writes on a sparse file
> larger than 4G.
>
> Checked on x86_64, i686, x32, aarch64, armhf, and s390.
>
>         * sysdeps/unix/sysv/linux/sysdep.h
>         [__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): Remove
>         guards.
>         * misc/tst-preadvwritev-common.c: New file.
>         * misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c.
>         * misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and add
>         a check for files larger than 2GB.
>
> diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
> index 8c9e62e..a469f57 100644
> --- a/sysdeps/unix/sysv/linux/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sysdep.h
> @@ -49,10 +49,6 @@
>  #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) \
> -  (long) (val), \
> -  (long) (((uint64_t) (val)) >> 32)
> -#endif
> +#define LO_HI_LONG(val) \
> + (long) (val), \
> + (long) (((uint64_t) (val)) >> 32)
> --

This change is incorrect for x32.  __ASSUME_WORDSIZE64_ILP32 seems
to mean different things for mips and x86-64. X86-64 always passes 64-bit
value in a 64-bit register.
  
Chris Metcalf July 11, 2016, 9:28 p.m. UTC | #3
On 7/11/2016 5:09 PM, H.J. Lu wrote:
> On Wed, Jul 6, 2016 at 12:52 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> The p{read,write}v{64} consolidation patch [1] added a wrong guard
>> for LO_HI_LONG definition.  It currently uses both
>> '__WORDSIZE == 64' and 'defined __ASSUME_WORDSIZE64_ILP32' to set
>> the value to be passed in one argument, otherwise it will be split
>> in two.  Since kernel commit 601cc11d054 the non-compat
>> preadv/pwritev in same order, so the LO_HI_LONG is the same regardless
>> off/off64_t size or wordsize.
>>
>> The tests were passing on 64-bit because files were small enough so
>> the high part of offset is 0 regardless.  This patch also changes the
>> tst-preadvwritev64 test to check reads and writes on a sparse file
>> larger than 4G.
>>
>> Checked on x86_64, i686, x32, aarch64, armhf, and s390.
>>
>>          * sysdeps/unix/sysv/linux/sysdep.h
>>          [__WORDSIZE == 64 || __ASSUME_WORDSIZE64_ILP32] (LO_HI_LONG): Remove
>>          guards.
>>          * misc/tst-preadvwritev-common.c: New file.
>>          * misc/tst-preadvwritev.c: Use tst-preadvwritev-common.c.
>>          * misc/tst-preadvwritev64.c: Use tst-preadwritev-common.c and add
>>          a check for files larger than 2GB.
>>
>> diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
>> index 8c9e62e..a469f57 100644
>> --- a/sysdeps/unix/sysv/linux/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/sysdep.h
>> @@ -49,10 +49,6 @@
>>   #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) \
>> -  (long) (val), \
>> -  (long) (((uint64_t) (val)) >> 32)
>> -#endif
>> +#define LO_HI_LONG(val) \
>> + (long) (val), \
>> + (long) (((uint64_t) (val)) >> 32)
>> --
> This change is incorrect for x32.  __ASSUME_WORDSIZE64_ILP32 seems
> to mean different things for mips and x86-64. X86-64 always passes 64-bit
> value in a 64-bit register.

Also, I'm not convinced the old code was right either.

In the 64-bit case (or HJ's 32-bit with 64-bit syscall args), shouldn't we
have "#define LO_HI_LONG(val) (val), 0"?  The trailing zero will ensure that
the kernel's pos_h argument gets a zero value so we don't end up
or'ing in garbage with the pos_from_hilo() call.
  

Patch

diff --git a/misc/tst-preadvwritev-common.c b/misc/tst-preadvwritev-common.c
new file mode 100644
index 0000000..9a66a5f
--- /dev/null
+++ b/misc/tst-preadvwritev-common.c
@@ -0,0 +1,112 @@ 
+/* Common definitions 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>
+
+static void do_prepare (void);
+#define PREPARE(argc, argv)     do_prepare ()
+static int do_test (void);
+#define TEST_FUNCTION           do_test ()
+#include "test-skeleton.c"
+
+static char *temp_filename;
+static int temp_fd;
+
+static 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)
+
+static int
+do_test_with_offset (off_t offset)
+{
+  struct iovec iov[2];
+  ssize_t ret;
+
+  char buf1[32];
+  char buf2[64];
+
+  memset (buf1, 0xf0, sizeof buf1);
+  memset (buf2, 0x0f, sizeof buf2);
+
+  /* Write two buffer with 32 and 64 bytes respectively.  */
+  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, offset);
+  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 + offset);
+  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;
+
+  /* Now read two buffer with 32 and 64 bytes respectively.  */
+  ret = preadv (temp_fd, iov, 2, offset);
+  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 + offset);
+  if (ret == -1)
+    FAIL ("second preadv returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("second preadv returned an unexpected value");
+
+  /* And compare the buffers read and written to check if there are equal.  */
+  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-preadvwritev.c b/misc/tst-preadvwritev.c
index 08deecc..50a80a7 100644
--- a/misc/tst-preadvwritev.c
+++ b/misc/tst-preadvwritev.c
@@ -16,99 +16,10 @@ 
    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)
+#include "tst-preadvwritev-common.c"
 
 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;
+  return do_test_with_offset (0);
 }
diff --git a/misc/tst-preadvwritev64.c b/misc/tst-preadvwritev64.c
index ff6e134..0e1f341 100644
--- a/misc/tst-preadvwritev64.c
+++ b/misc/tst-preadvwritev64.c
@@ -16,7 +16,27 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#define PREADV  preadv64
-#define PWRITEV pwritev64
+#define _FILE_OFFSET_BITS 64
+#include "tst-preadvwritev-common.c"
 
-#include "tst-preadvwritev.c"
+int
+do_test (void)
+{
+  int ret;
+
+  ret = do_test_with_offset (0);
+
+  /* Create a sparse file larger than 4GB to check if offset is handled
+     correctly in p{write,read}v64. */
+  off_t base_offset = UINT32_MAX;
+  off_t fsize = base_offset + 2048;
+  if (ftruncate (temp_fd, fsize) != 0)
+    {
+      printf ("error: ftruncate (%jd) failed: %m", (intmax_t) fsize);
+      return 1;
+    }
+
+  ret += do_test_with_offset (base_offset);
+
+  return ret;
+}
diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index 8c9e62e..a469f57 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -49,10 +49,6 @@ 
 #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) \
-  (long) (val), \
-  (long) (((uint64_t) (val)) >> 32)
-#endif
+#define LO_HI_LONG(val) \
+ (long) (val), \
+ (long) (((uint64_t) (val)) >> 32)