Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

On 08/24/2017 08:03 AM, Richard Biener wrote:
> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>> test triggered by a recent VRP improvement.  A simple test case
>> that approximates the warning is:
>>
>>   void f (char *d, const char *s, size_t n)
>>   {
>>     if (n > 0 && n <= SIZE_MAX / 2)
>>       n = 0;
>>
>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>   }
>>
>> Since the only valid value of n is zero the call to memcpy can
>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>> the maximum size of the largest object and would surely make
>> the call crash).
>>
>> The important difference between the test case and Bug 81908
>> is that in the latter, the code is emitted by GCC itself from
>> what appears to be correct source (looks like it's the result
>> of the loop distribution pass).  I believe the warning for
>> the test case above and for other human-written code like it
>> is helpful, but warning for code emitted by GCC, even if it's
>> dead or unreachable, is obviously not (at least not to users).
>>
>> The attached patch enhances the gimple_fold_builtin_memory_op
>> function to eliminate this patholohgical case by making use
>> of range information to fold into no-ops calls to memcpy whose
>> size argument is in a range where the only valid value is zero.
>> This gets rid of the warning and improves the emitted code.
>>
>> Tested on x86_64-linux.
>
> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
>    tree destvar, srcvar;
>    location_t loc = gimple_location (stmt);
>
> +  if (tree valid_len = only_valid_value (len))
> +    {
> +      /* LEN is in range whose only valid value is zero.  */
> +      len = valid_len;
> +    }
> +
>    /* If the LEN parameter is zero, return DEST.  */
>    if (integer_zerop (len))
>
> just enhance this check to
>
>   if (integer_zerop (len)
>       || size_must_be_zero_p (len))
>
> ?  'only_valid_value' looks too generic for this.

Sure.

FWIW, the reason I did it this was because my original patch
returned error_mark_node for entirely invalid ranges and had
the caller replace the call with a trap.  I decided not to
include that part in this fix to keep it contained.

>
> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
> +    return NULL_TREE;
> +
>
> why?

Only because I never remember what APIs are safe to use with
what input.

> +  if (wi::eq_p (min, wone)
> +      && wi::geu_p (max + 1, ssize_max))
>
>    if (wi::eq_p (min, 1)
>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>
> your ssize_max isn't signed size max, and max + 1 might overflow to zero.

You're right that the addition to max would be better done
as subtraction from the result of (1 << N).  Thank you.

If (max + 1) overflowed then (max == TYPE_MAX) would have
to hold which I thought could never be true for an anti
range. (The patch includes tests for this case.)  Was I
wrong?

Attached is an updated version with the suggested changes
plus an additional test to verify the absence of warnings.

Martin

[-- Attachment #2: gcc-81908.diff --]
[-- Type: text/x-patch, Size: 7024 bytes --]

PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32

gcc/ChangeLog:

	PR middle-end/81908
	* gimple-fold.c (size_must_be_zero_p): New function.
	(gimple_fold_builtin_memory_op): Call it.

gcc/testsuite/ChangeLog:

	PR middle-end/81908
	* gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
	* gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.
	* gcc.dg/tree-ssa/pr81908.c: New test.

diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 251446c..c4184fa 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -628,6 +628,36 @@ var_decl_component_p (tree var)
   return SSA_VAR_P (inner);
 }
 
+/* If the SIZE argument representing the size of an object is in a range
+   of values of which exactly one is valid (and that is zero), return
+   true, otherwise false.  */
+
+static bool
+size_must_be_zero_p (tree size)
+{
+  if (integer_zerop (size))
+    return true;
+
+  if (TREE_CODE (size) != SSA_NAME)
+    return false;
+
+  wide_int min, max;
+  enum value_range_type rtype = get_range_info (size, &min, &max);
+  if (rtype != VR_ANTI_RANGE)
+    return false;
+
+  tree type = TREE_TYPE (size);
+  int prec = TYPE_PRECISION (type);
+
+  wide_int wone = wi::one (prec);
+
+  /* Compute the value of SSIZE_MAX, the largest positive value that
+     can be stored in ssize_t, the signed counterpart of size_t .  */
+  wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
+
+  return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max);
+}
+
 /* Fold function call to builtin mem{{,p}cpy,move}.  Return
    false if no simplification can be made.
    If ENDP is 0, return DEST (like memcpy).
@@ -646,8 +676,9 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
   tree destvar, srcvar;
   location_t loc = gimple_location (stmt);
 
-  /* If the LEN parameter is zero, return DEST.  */
-  if (integer_zerop (len))
+  /* If the LEN parameter is a constant zero or in range where
+     the only valid value is zero, return DEST.  */
+  if (size_must_be_zero_p (len))
     {
       gimple *repl;
       if (gimple_call_lhs (stmt))
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
new file mode 100644
index 0000000..5bdb342
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
@@ -0,0 +1,44 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   Test to verify that calls to memcpy et al. where the size is in a range
+   with just one valid value -- zero -- are eliminated.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define INT_MAX    __INT_MAX__
+#define SHRT_MAX   __SHRT_MAX__
+#define SIZE_MAX   __SIZE_MAX__
+#define SSIZE_MAX  (SIZE_MAX / 2)
+
+typedef __PTRDIFF_TYPE__ ssize_t;
+typedef __SIZE_TYPE__    size_t;
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, 1, SHRT_MAX, 0);
+AR (memcpy, int,   1, INT_MAX, 0);
+AR (memcpy, size_t,  1, SSIZE_MAX, 0);
+AR (memcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (memmove, short, 1, SHRT_MAX, 0);
+AR (memmove, int,   1, INT_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (mempcpy, short, 1, SHRT_MAX, 0);
+AR (mempcpy, int,   1, INT_MAX, 0);
+AR (mempcpy, size_t,  1, SSIZE_MAX, 0);
+AR (mempcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+/* { dg-final { scan-tree-dump-not "builtin_memcpy" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_memmove" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_mempcpy" "optimized" } }  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
new file mode 100644
index 0000000..716be5b8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
@@ -0,0 +1,43 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   Test to verify that calls to memcpy et al. where the size is in a range
+   with more than one valid value are not eliminated (this test complements
+   builtins-folding-gimple-2.c).
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define SHRT_MAX   __SHRT_MAX__
+#define SHRT_MIN   (-SHRT_MAX - 1)
+#define INT_MAX    __INT_MAX__
+#define INT_MIN    (-INT_MAX - 1)
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, SHRT_MIN, 0, 1);
+AR (memcpy, short, SHRT_MIN, 1, 2);
+AR (memcpy, short, 2, SHRT_MAX, 1);
+
+AR (memcpy, int, INT_MIN, 0, 1);
+AR (memcpy, int, INT_MIN, 1, 2);
+AR (memcpy, int, INT_MIN, 2, 3);
+AR (memcpy, int, 2, INT_MAX, 1);
+AR (memcpy, int, 2, INT_MAX, 1);
+
+AR (memmove, short, 2, SHRT_MAX, 1);
+AR (memmove, int,   2, INT_MAX, 1);
+
+AR (mempcpy, short, 2, SHRT_MAX, 1);
+AR (mempcpy, int,   2, INT_MAX, 1);
+
+/* { dg-final { scan-tree-dump-times "builtin_memcpy" 8 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_memmove" 2 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_mempcpy" 2 "optimized" } }  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c b/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c
new file mode 100644
index 0000000..b1e316a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c
@@ -0,0 +1,46 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+#define SIZE_MAX __SIZE_MAX__
+typedef __SIZE_TYPE__ size_t;
+
+void f0 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2 - 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}
+
+void f1 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);   /* { dg-bogus "\\\[-Wstringop-overflow=]" } */
+}
+
+void f2 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2 + 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}
+
+void f3 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX - 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);   /* { dg-bogus "\\\[-Wstringop-overflow=]" } */
+}
+
+void f4 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}