diff mbox

tile: Check for pointer add overflow in memchr

Message ID 3a3705d5-6cc0-58c7-4435-fb73abedcbe3@mellanox.com
State New
Headers show

Commit Message

Chris Metcalf Jan. 13, 2017, 8:06 p.m. UTC 
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 mbox

Patch

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);