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


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

Re: [PATCH] PowerPC64 - Fix INTERNAL_[V]SYSCALL_NCS macros to notcast return val to (int)


On Mon, 2011-02-14 at 10:57 -0800, Roland McGrath wrote:
> > Corrected the formatting.
> 
> Some of it is now correct, but plenty is still wrong.

Corrected: Text width, removed comment line leading *, added space
between function and (), added spaces between assignments variables,
added space between variables and arithmetic operators, added spaces
between parameters, replaced fopen with open.

> > Fixed, though I have a question, what's the difference between
> > UIO_MAXIOV and IOV_MAX?  POSIX specifies using IOV_MAX in the writev
> > documentation but I used UIO_MAXIOV as you indicated.
> 
> IOV_MAX is the proper POSIX name.  Use that.

Done.

> > /* writev should report that it has written EXPECTED number of bytes.  */
> > #define EXPECTED ((ssize_t)INT32_MAX+1)
> 
> You can make this size_t and then use a cast on the return value to avoid
> the compiler warning in the comparison.

Done.

> > /* How big does each buffer have to be to fit EXPECTED into UIO_MAXIOV iovec
> >  * structs?  We do this because not all systems will support an EXPECTED size
> >  * malloc.  */
> > const ssize_t bufsz = (EXPECTED / (UIO_MAXIOV - 1));
> > 
> > /* The last buffer is only partially full.  */
> > const ssize_t bufrem = (EXPECTED % (UIO_MAXIOV - 1));

> These should be size_t and there is no reason to make them globals.

Done, made them locals.

> Why - 1?

POSIX doesn't guarantee that IOV_MAX is power of 2 so my bufsz and
bufrem calculations try to fit as much as possible into UIO_MAX - 1
buffers, and then the remainder into the last iovec buffer in the array.

The above method had quite a bit of wasted space in the last iovec
buffer so I've come up with the following which is optimistic and
presumes that IOV_MAX is pow of 2.  If it isn't we just make bufsz one
byte larger.  Then the remainder fills almost the complete last buffer.

+  /* POSIX doesn't guarantee that IOV_MAX is pow of 2 but we're optimistic.  */
+  size_t bufsz = (EXPECTED / IOV_MAX);
+  size_t bufrem = (EXPECTED % IOV_MAX);
+
+  /* If there's a remainder then IOV_MAX probably isn't a power of 2 and we
+     need to make bufsz bigger so that the last iovec, iv[IOV_MAX-1], is free
+     for the remainder.  */
+  if (bufrem)
+    {
+      bufsz = bufsz + 1;
+      bufrem = (EXPECTED - (bufsz * (IOV_MAX - 1)));
+    }

The above code has been tested with non power-of-2 IOV_MAX by
uncommenting the #if 0 block in the test case which undefines IOV_MAX
and redefines it to a non power-of-2 value.

Ryan S. Arnold

2011-02-15  Ryan S. Arnold  <rsa@us.ibm.com>

        * sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h:
        (INTERNAL_VSYSCALL_NCS INTERNAL_SYSCALL_NCS): Remove erroneous (int)
        cast from r3.
        * sysdeps/wordsize-64/Makefile: New file that adds tst-writev to
        'tests' variable.
        * sysdeps/wordsize-64/tst-writev.c: New file that verifies that
        writev() returns the valid number of written bytes in the return
        value, i.e., it makes sure the ret val isn't mangled by the syscall
        macros.

commit 6a0494d4f3a576729f155836faca39c4989bfa87
Author: Ryan S. Arnold <rsa@us.ibm.com>
Date:   Tue Feb 15 13:29:21 2011 -0600

    Remove erroneous (int) cast from PowerPC64 INTERNAL_[V]SYSCALL_NCS macros.
    
    The PowerPC64 implementation of INTERNAL_[V]SYSCALL_NCS erroneously casts
    the return value held in r3 to (int).  Normally this wasn't a problem but
    for syscalls wrappers that require a ssize_t return, like writev() this bug
    would mangle the high half word of the return value and even erroneously
    sign-extend the high half word if the sign-bit of the low half word was '1'.
    This patch also adds a testcase which proves correct behavior for
    wordsize-64.

diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
index aab4b72..e714c4c 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h
@@ -172,7 +172,7 @@
        : "r9", "r10", "r11", "r12",					\
          "cr0", "ctr", "lr", "memory");					\
 	  err = (long int) r0;						\
-    (int) r3;								\
+    r3;								\
   })
 
 #undef INLINE_SYSCALL
@@ -219,7 +219,7 @@
        : "r9", "r10", "r11", "r12",					\
          "cr0", "ctr", "memory");					\
 	  err = r0;  \
-    (int) r3;  \
+    r3;  \
   })
 #define INTERNAL_SYSCALL(name, err, nr, args...)			\
   INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)
diff --git a/sysdeps/wordsize-64/Makefile b/sysdeps/wordsize-64/Makefile
new file mode 100644
index 0000000..9903f51
--- /dev/null
+++ b/sysdeps/wordsize-64/Makefile
@@ -0,0 +1,6 @@
+ifeq ($(subdir),misc)
+tests += tst-writev
+
+# Time enough for a large writev syscall to complete.
+tst-writev-ENV = TIMEOUTFACTOR="10"
+endif
diff --git a/sysdeps/wordsize-64/tst-writev.c b/sysdeps/wordsize-64/tst-writev.c
new file mode 100644
index 0000000..8833834
--- /dev/null
+++ b/sysdeps/wordsize-64/tst-writev.c
@@ -0,0 +1,103 @@
+/* Copyright (C) 2011 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ryan S. Arnold <rsa@us.ibm.com>, 2011.
+
+   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, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/uio.h>
+
+#include <errno.h>
+
+/* The purpose of this test is to verify that the INTERNAL_[V]SYSCALL_NCS
+   macros on 64-bit platforms don't cast the return type to (int) which would
+   erroneously sign extend the return value should the high bit of the bottom
+   half of the word be '1'.  */
+
+#if 0
+/* Used to test the non power-of-2 code path.  */
+#undef IOV_MAX
+#define IOV_MAX 1000
+#endif
+
+/* writev() should report that it has written EXPECTED number of bytes.  */
+#define EXPECTED (((size_t) INT32_MAX) + 1)
+
+static int
+do_test (void)
+{
+  int fd;
+  struct iovec iv[IOV_MAX];
+  ssize_t ret;
+  int i;
+  /* POSIX doesn't guarantee that IOV_MAX is pow of 2 but we're optimistic.  */
+  size_t bufsz = (EXPECTED / IOV_MAX);
+  size_t bufrem = (EXPECTED % IOV_MAX);
+
+  /* If there's a remainder then IOV_MAX probably isn't a power of 2 and we
+     need to make bufsz bigger so that the last iovec, iv[IOV_MAX-1], is free
+     for the remainder.  */
+  if (bufrem)
+    {
+      bufsz = bufsz + 1;
+      bufrem = (EXPECTED - (bufsz * (IOV_MAX - 1)));
+    }
+
+  /* We writev to /dev/null since we're just testing writev's return value.  */
+  if ((fd = open ("/dev/null", O_WRONLY)) == -1)
+    {
+      printf ("Unable to open /dev/null for appending.\n");
+      return -1;
+    }
+
+  iv[0].iov_base = malloc (bufsz);
+  iv[0].iov_len = bufsz;
+
+  /* We optimistically presume that there isn't a remainder and set all iovec
+     instances to the same base and len as the first instance.  */
+  for (i = 1; i < IOV_MAX; i++)
+    {
+      /* We don't care what the data is so reuse the allocation from iv[0];  */
+      iv[i].iov_base = iv[0].iov_base;
+      iv[i].iov_len = iv[0].iov_len;
+    }
+
+  /* If there is a remainder then we correct the last iov_len.  */
+  if (bufrem)
+    iv[IOV_MAX - 1].iov_len = bufrem;
+
+  /* Write junk to /dev/null with the writev syscall in order to get a return
+     of INT32_MAX+1 bytes to verify that the INTERNAL_SYSCALL wrappers aren't
+     mangling the result if the signbit of a 32-bit number is set.  */
+  ret = writev (fd, iv, IOV_MAX);
+
+  free (iv[0].iov_base);
+  close (fd);
+
+  if (ret != (ssize_t) EXPECTED)
+    {
+      printf ("writev() return value: %zd != EXPECTED: %zd\n", ret, EXPECTED);
+      return ret;
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"


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