Re: [PATCH] tile: Check for pointer add overflow in memchr

[Chris Metcalf <cmetcalf at mellanox dot com>]

On 1/13/2017 5:42 AM, Adhemerval Zanella wrote:
> On 12/01/2017 19:37, Chris Metcalf wrote:
> > +  /* Handle possible addition overflow.  */
> > +  if (__glibc_unlikely ((unsigned long) last_byte_ptr < (unsigned long) s))
> > +    last_byte_ptr = (const char *) UINTPTR_MAX;
> > +
> Wouldn't a branchfree saturating addition be better for tile?

Actually, it turns out that branching here is pretty reasonable.
Due to the way tilegx bundles instructions, we only add a single
bundle as a result of doing the comparison and branch.  (If we
actually take the branch, that adds a couple of extra cycles, but I
think this is a pretty unlikely case, so we don't really care.)

We could use a cmov instruction to avoid the branch, but that
actually takes an additional cycle since there are more dependencies,
so the branch-taken case gets faster at the expense of the
branch-not-taken case, which seems like the wrong thing.

You're right that with the 32-bit case (either tilegx32 or tilepro)
we can use a saturating add instruction directly, which complexifies
the tilegx code a bit with #ifdefs, but we might as well do, I think.
Unfortunately, we don't have a 64-bit saturating add instruction.


diff --git a/sysdeps/tile/tilegx/memchr.c b/sysdeps/tile/tilegx/memchr.c
index 34df19d2319c..e85d3304cf9a 100644
--- a/sysdeps/tile/tilegx/memchr.c
+++ b/sysdeps/tile/tilegx/memchr.c
@@ -20,6 +20,17 @@
 #include <stdint.h>
 #include "string-endian.h"

+static uintptr_t
+saturating_add (uintptr_t p, size_t n)
+{
+#ifdef _LP64
+  uintptr_t result = p + n;
+  return __glibc_likely (result >= p) ? result : UINTPTR_MAX;
+#else
+  return __insn_addxsc (p, n);
+#endif
+}
+
 void *
 __memchr (const void *s, int c, size_t n)
 {
@@ -49,7 +60,7 @@ __memchr (const void *s, int c, size_t n)
   v = (*p | before_mask) ^ (goal & before_mask);

   /* Compute the address of the last byte. */
-  last_byte_ptr = (const char *) s + n - 1;
+  last_byte_ptr = (const char *) saturating_add ((uintptr_t) s, n - 1);

   /* Compute the address of the word containing the last byte. */
   last_word_ptr = (const uint64_t *) ((uintptr_t) last_byte_ptr & -8);
diff --git a/sysdeps/tile/tilepro/memchr.c b/sysdeps/tile/tilepro/memchr.c
index 1848a9cadb2d..500a8940ac34 100644
--- a/sysdeps/tile/tilepro/memchr.c
+++ b/sysdeps/tile/tilepro/memchr.c
@@ -48,8 +48,8 @@ __memchr (const void *s, int c, size_t n)
   before_mask = (1 << (s_int << 3)) - 1;
   v = (*p | before_mask) ^ (goal & before_mask);

-  /* Compute the address of the last byte. */
-  last_byte_ptr = (const char *) s + n - 1;
+  /* Compute the address of the last byte using a saturating add. */
+  last_byte_ptr = (const char *) __insn_adds ((uintptr_t) s, n - 1);

   /* Compute the address of the word containing the last byte. */
   last_word_ptr = (const uint32_t *) ((uintptr_t) last_byte_ptr & -4);

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