Avoid UB in ia32intrin.h rotate patterns (PR target/82498)

Message ID 20171012193940.GB14653@tucnak
State New
Headers show
Series
  • Avoid UB in ia32intrin.h rotate patterns (PR target/82498)
Related show

Commit Message

Jakub Jelinek Oct. 12, 2017, 7:39 p.m.
Hi!

The ia32intrin.h rotate intrinsics require the second argument to be
in between 1 and 31 (or 63), otherwise they invoke UB.  But, we can do much
better while generating the same instruction when optimizing, so the
following patch uses the patterns we pattern recognize well and where
the second argument can be any value.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-10-12  Jakub Jelinek  <jakub@redhat.com>

	PR target/82498
	* config/i386/ia32intrin.h (__rold, __rord, __rolq, __rorq): Allow
	any values of __C while still being pattern recognizable as a simple
	rotate instruction.

	* gcc.dg/ubsan/pr82498.c: New test.


	Jakub

Comments

Uros Bizjak Oct. 13, 2017, 6:48 a.m. | #1
On Thu, Oct 12, 2017 at 9:39 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The ia32intrin.h rotate intrinsics require the second argument to be
> in between 1 and 31 (or 63), otherwise they invoke UB.  But, we can do much
> better while generating the same instruction when optimizing, so the
> following patch uses the patterns we pattern recognize well and where
> the second argument can be any value.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-10-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/82498
>         * config/i386/ia32intrin.h (__rold, __rord, __rolq, __rorq): Allow
>         any values of __C while still being pattern recognizable as a simple
>         rotate instruction.
>
>         * gcc.dg/ubsan/pr82498.c: New test.

LGTM.

Thanks,
Uros.

> --- gcc/config/i386/ia32intrin.h.jj     2017-01-01 12:45:42.000000000 +0100
> +++ gcc/config/i386/ia32intrin.h        2017-10-12 09:55:24.235602737 +0200
> @@ -147,7 +147,8 @@ extern __inline unsigned int
>  __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>  __rold (unsigned int __X, int __C)
>  {
> -  return (__X << __C) | (__X >> (32 - __C));
> +  __C &= 31;
> +  return (__X << __C) | (__X >> (-__C & 31));
>  }
>
>  /* 8bit ror */
> @@ -171,7 +172,8 @@ extern __inline unsigned int
>  __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>  __rord (unsigned int __X, int __C)
>  {
> -  return (__X >> __C) | (__X << (32 - __C));
> +  __C &= 31;
> +  return (__X >> __C) | (__X << (-__C & 31));
>  }
>
>  /* Pause */
> @@ -239,7 +241,8 @@ extern __inline unsigned long long
>  __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>  __rolq (unsigned long long __X, int __C)
>  {
> -  return (__X << __C) | (__X >> (64 - __C));
> +  __C &= 63;
> +  return (__X << __C) | (__X >> (-__C & 63));
>  }
>
>  /* 64bit ror */
> @@ -247,7 +250,8 @@ extern __inline unsigned long long
>  __attribute__((__gnu_inline__, __always_inline__, __artificial__))
>  __rorq (unsigned long long __X, int __C)
>  {
> -  return (__X >> __C) | (__X << (64 - __C));
> +  __C &= 63;
> +  return (__X >> __C) | (__X << (-__C & 63));
>  }
>
>  /* Read flags register */
> --- gcc/testsuite/gcc.dg/ubsan/pr82498.c.jj     2017-10-12 09:40:36.025438511 +0200
> +++ gcc/testsuite/gcc.dg/ubsan/pr82498.c        2017-10-12 10:06:06.636790077 +0200
> @@ -0,0 +1,159 @@
> +/* PR target/82498 */
> +/* { dg-do run { target i?86-*-* x86_64-*-* } } */
> +/* { dg-options "-fsanitize=undefined -fno-sanitize-recover=undefined" } */
> +
> +#include <x86intrin.h>
> +
> +volatile unsigned int a;
> +volatile unsigned long long b;
> +volatile int c;
> +
> +int
> +main ()
> +{
> +  a = 0x12345678U;
> +  a = __rold (a, 0);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rold (a, 32);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rold (a, -32);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rold (a, 37);
> +  if (a != 0x468acf02U)
> +    __builtin_abort ();
> +  a = __rold (a, -5);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rord (a, 0);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rord (a, 32);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rord (a, -32);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  a = __rord (a, -37);
> +  if (a != 0x468acf02U)
> +    __builtin_abort ();
> +  a = __rord (a, 5);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = 0;
> +  a = __rold (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = 32;
> +  a = __rold (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = -32;
> +  a = __rold (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = 37;
> +  a = __rold (a, c);
> +  if (a != 0x468acf02U)
> +    __builtin_abort ();
> +  c = -5;
> +  a = __rold (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = 0;
> +  a = __rord (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = 32;
> +  a = __rord (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = -32;
> +  a = __rord (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +  c = -37;
> +  a = __rord (a, c);
> +  if (a != 0x468acf02U)
> +    __builtin_abort ();
> +  c = 5;
> +  a = __rord (a, c);
> +  if (a != 0x12345678U)
> +    __builtin_abort ();
> +#ifdef __x86_64__
> +  b = 0x123456789abcdef1ULL;
> +  b = __rolq (b, 0);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rolq (b, 64);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rolq (b, -64);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rolq (b, 69);
> +  if (b != 0x468acf13579bde22ULL)
> +    __builtin_abort ();
> +  b = __rolq (b, -5);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rorq (b, 0);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rorq (b, 64);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rorq (b, -64);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  b = __rorq (b, -69);
> +  if (b != 0x468acf13579bde22ULL)
> +    __builtin_abort ();
> +  b = __rorq (b, 5);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = 0;
> +  b = __rolq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = 64;
> +  b = __rolq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = -64;
> +  b = __rolq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = 69;
> +  b = __rolq (b, c);
> +  if (b != 0x468acf13579bde22ULL)
> +    __builtin_abort ();
> +  c = -5;
> +  b = __rolq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = 0;
> +  b = __rorq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = 64;
> +  b = __rorq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = -64;
> +  b = __rorq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +  c = -69;
> +  b = __rorq (b, c);
> +  if (b != 0x468acf13579bde22ULL)
> +    __builtin_abort ();
> +  c = 5;
> +  b = __rorq (b, c);
> +  if (b != 0x123456789abcdef1ULL)
> +    __builtin_abort ();
> +#endif
> +  return 0;
> +}
>
>         Jakub

Patch

--- gcc/config/i386/ia32intrin.h.jj	2017-01-01 12:45:42.000000000 +0100
+++ gcc/config/i386/ia32intrin.h	2017-10-12 09:55:24.235602737 +0200
@@ -147,7 +147,8 @@  extern __inline unsigned int
 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __rold (unsigned int __X, int __C)
 {
-  return (__X << __C) | (__X >> (32 - __C));
+  __C &= 31;
+  return (__X << __C) | (__X >> (-__C & 31));
 }
 
 /* 8bit ror */
@@ -171,7 +172,8 @@  extern __inline unsigned int
 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __rord (unsigned int __X, int __C)
 {
-  return (__X >> __C) | (__X << (32 - __C));
+  __C &= 31;
+  return (__X >> __C) | (__X << (-__C & 31));
 }
 
 /* Pause */
@@ -239,7 +241,8 @@  extern __inline unsigned long long
 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __rolq (unsigned long long __X, int __C)
 {
-  return (__X << __C) | (__X >> (64 - __C));
+  __C &= 63;
+  return (__X << __C) | (__X >> (-__C & 63));
 }
 
 /* 64bit ror */
@@ -247,7 +250,8 @@  extern __inline unsigned long long
 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __rorq (unsigned long long __X, int __C)
 {
-  return (__X >> __C) | (__X << (64 - __C));
+  __C &= 63;
+  return (__X >> __C) | (__X << (-__C & 63));
 }
 
 /* Read flags register */
--- gcc/testsuite/gcc.dg/ubsan/pr82498.c.jj	2017-10-12 09:40:36.025438511 +0200
+++ gcc/testsuite/gcc.dg/ubsan/pr82498.c	2017-10-12 10:06:06.636790077 +0200
@@ -0,0 +1,159 @@ 
+/* PR target/82498 */
+/* { dg-do run { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-fsanitize=undefined -fno-sanitize-recover=undefined" } */
+
+#include <x86intrin.h>
+
+volatile unsigned int a;
+volatile unsigned long long b;
+volatile int c;
+
+int
+main ()
+{
+  a = 0x12345678U;
+  a = __rold (a, 0);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rold (a, 32);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rold (a, -32);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rold (a, 37);
+  if (a != 0x468acf02U)
+    __builtin_abort ();
+  a = __rold (a, -5);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rord (a, 0);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rord (a, 32);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rord (a, -32);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  a = __rord (a, -37);
+  if (a != 0x468acf02U)
+    __builtin_abort ();
+  a = __rord (a, 5);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = 0;
+  a = __rold (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = 32;
+  a = __rold (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = -32;
+  a = __rold (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = 37;
+  a = __rold (a, c);
+  if (a != 0x468acf02U)
+    __builtin_abort ();
+  c = -5;
+  a = __rold (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = 0;
+  a = __rord (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = 32;
+  a = __rord (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = -32;
+  a = __rord (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+  c = -37;
+  a = __rord (a, c);
+  if (a != 0x468acf02U)
+    __builtin_abort ();
+  c = 5;
+  a = __rord (a, c);
+  if (a != 0x12345678U)
+    __builtin_abort ();
+#ifdef __x86_64__
+  b = 0x123456789abcdef1ULL;
+  b = __rolq (b, 0);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rolq (b, 64);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rolq (b, -64);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rolq (b, 69);
+  if (b != 0x468acf13579bde22ULL)
+    __builtin_abort ();
+  b = __rolq (b, -5);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rorq (b, 0);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rorq (b, 64);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rorq (b, -64);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  b = __rorq (b, -69);
+  if (b != 0x468acf13579bde22ULL)
+    __builtin_abort ();
+  b = __rorq (b, 5);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = 0;
+  b = __rolq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = 64;
+  b = __rolq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = -64;
+  b = __rolq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = 69;
+  b = __rolq (b, c);
+  if (b != 0x468acf13579bde22ULL)
+    __builtin_abort ();
+  c = -5;
+  b = __rolq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = 0;
+  b = __rorq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = 64;
+  b = __rorq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = -64;
+  b = __rorq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+  c = -69;
+  b = __rorq (b, c);
+  if (b != 0x468acf13579bde22ULL)
+    __builtin_abort ();
+  c = 5;
+  b = __rorq (b, c);
+  if (b != 0x123456789abcdef1ULL)
+    __builtin_abort ();
+#endif
+  return 0;
+}