[PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Martin Sebor]

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

The gimple_fold_builtin_memory_op() function folds calls to memcpy
and similar to MEM_REF when the size of the copy is a small power
of 2, but it does so without considering whether the copy might
write (or read) past the end of one of the objects.  To detect
these kinds of errors (and help distinguish them from -Westrict)
the folder calls into the wrestrict pass and lets it diagnose them.
Unfortunately, that can lead to false positives for even some fairly
straightforward code that is ultimately found to be unreachable.
PR 88800 is a report of one such problem.

To avoid these false positives the attached patch adjusts
the function to avoid issuing -Warray-bounds for out-of-bounds
calls to memcpy et al.  Instead, the patch disables the folding
of such invalid calls (and only those).  Those that are not
eliminated during DCE or other subsequent passes are eventually
diagnosed by the wrestrict pass.

Since this change required removing the dependency of the detection
on the warning options (originally done as a micro-optimization to
avoid spending compile-time cycles on something that wasn't needed)
the patch also adds tests to verify that code generation is not
affected as a result of warnings being enabled or disabled.  With
the patch as is, the invalid memcpy calls end up emitted (currently
they are folded into equally invalid MEM_REFs).  At some point,
I'd like us to consider whether they should be replaced with traps
(possibly under the control of  as has been proposed a number of
times in the past.  If/when that's done, these tests will need to
be adjusted to look for traps instead.

Tested on x86_64-linux.

Martin

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

PR tree-optimization/88800 - Spurious -Werror=array-bounds for non-taken branch

gcc/ChangeLog:

	PR tree-optimization/88800
	* gimple-fold.c (gimple_fold_builtin_memory_op): Avoid checking
	NO_WARNING bit here.  Avoid folding out-of-bounds calls.
	* gimple-ssa-warn-restrict.c (maybe_diag_offset_bounds): Remove
	redundant argument.  Add new argument and issue diagnostics under
	its control.  Detect out-of-bounds access even with warnings
	disabled.
	(check_bounds_or_overlap): Change return type.  Add argument.
	(wrestrict_dom_walker::check_call): Adjust.
	* gimple-ssa-warn-restrict.h (check_bounds_or_overlap): Add argument.
	* tree-ssa-strlen.c (handle_builtin_strcpy): Adjust to change in
	check_bounds_or_overlap's return value.
	(handle_builtin_stxncpy): Same.
	(handle_builtin_strcat): Same.

gcc/testsuite/ChangeLog:

	PR tree-optimization/88800
	* c-c++-common/Wrestrict.c: Adjust.
	* gcc.dg/Warray-bounds-37.c: New test.
	* gcc.dg/builtin-memcpy-2.c: New test.
	* gcc.dg/builtin-memcpy.c: New test.

Index: gcc/gimple-fold.c
===================================================================
--- gcc/gimple-fold.c	(revision 267925)
+++ gcc/gimple-fold.c	(working copy)
@@ -697,8 +697,6 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato
   tree destvar, srcvar;
   location_t loc = gimple_location (stmt);
 
-  bool nowarn = gimple_no_warning_p (stmt);
-
   /* 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))
@@ -766,12 +764,16 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato
 	  unsigned ilen = tree_to_uhwi (len);
 	  if (pow2p_hwi (ilen))
 	    {
-	      /* Detect invalid bounds and overlapping copies and issue
-		 either -Warray-bounds or -Wrestrict.  */
-	      if (!nowarn
-		  && check_bounds_or_overlap (as_a <gcall *>(stmt),
-					      dest, src, len, len))
-	      	gimple_set_no_warning (stmt, true);
+	      /* Detect out-of-bounds accesses without issuing warnings.
+		 Avoid folding out-of-bounds copies but to avoid false
+		 positives for unreachable code defer warning until after
+		 DCE has worked its magic.
+		 -Wrestrict is still diagnosed.  */
+	      if (int warning = check_bounds_or_overlap (as_a <gcall *>(stmt),
+							 dest, src, len, len,
+							 false, false))
+		if (warning != OPT_Wrestrict)
+		  return false;
 
 	      scalar_int_mode mode;
 	      tree type = lang_hooks.types.type_for_size (ilen * 8, 1);
@@ -1038,10 +1040,16 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato
 	    }
 	}
 
-      /* Detect invalid bounds and overlapping copies and issue either
-	 -Warray-bounds or -Wrestrict.  */
-      if (!nowarn)
-	check_bounds_or_overlap (as_a <gcall *>(stmt), dest, src, len, len);
+      /* Same as above, detect out-of-bounds accesses without issuing
+	 warnings.  Avoid folding out-of-bounds copies but to avoid
+	 false positives for unreachable code defer warning until
+	 after DCE has worked its magic.
+	 -Wrestrict is still diagnosed.  */
+      if (int warning = check_bounds_or_overlap (as_a <gcall *>(stmt),
+						 dest, src, len, len,
+						 false, false))
+	if (warning != OPT_Wrestrict)
+	  return false;
 
       gimple *new_stmt;
       if (is_gimple_reg_type (TREE_TYPE (srcvar)))
Index: gcc/gimple-ssa-warn-restrict.c
===================================================================
--- gcc/gimple-ssa-warn-restrict.c	(revision 267925)
+++ gcc/gimple-ssa-warn-restrict.c	(working copy)
@@ -1329,6 +1329,9 @@ maybe_diag_overlap (location_t loc, gimple *call,
   if (!acs.overlap ())
     return false;
 
+  if (gimple_no_warning_p (call))
+    return true;
+
   /* For convenience.  */
   const builtin_memref &dstref = *acs.dstref;
   const builtin_memref &srcref = *acs.srcref;
@@ -1568,7 +1571,7 @@ maybe_diag_overlap (location_t loc, gimple *call,
   return true;
 }
 
-/* Validate REF offsets in an EXPRession passed as an argument to a CALL
+/* Validate REF offsets in an expression passed as an argument to a CALL
    to a built-in function FUNC to make sure they are within the bounds
    of the referenced object if its size is known, or PTRDIFF_MAX otherwise.
    Both initial values of the offsets and their final value computed by
@@ -1578,8 +1581,19 @@ maybe_diag_overlap (location_t loc, gimple *call,
 
 static bool
 maybe_diag_offset_bounds (location_t loc, gimple *call, tree func, int strict,
-			  tree expr, const builtin_memref &ref)
+			  const builtin_memref &ref, bool do_warn)
 {
+  /* Check for out-bounds pointers regardless of warning options since
+     the result is used to make codegen decisions.  */
+  offset_int ooboff[] = { ref.offrange[0], ref.offrange[1] };
+  tree oobref = ref.offset_out_of_bounds (strict, ooboff);
+  if (!oobref)
+    return false;
+
+  /* Return true without issuing a warning.  */
+  if (!do_warn)
+    return true;
+
   if (!warn_array_bounds)
     return false;
 
@@ -1586,14 +1600,9 @@ maybe_diag_offset_bounds (location_t loc, gimple *
   if (ref.ref && TREE_NO_WARNING (ref.ref))
     return false;
 
-  offset_int ooboff[] = { ref.offrange[0], ref.offrange[1] };
-  tree oobref = ref.offset_out_of_bounds (strict, ooboff);
-  if (!oobref)
-    return false;
+  if (EXPR_HAS_LOCATION (ref.ptr))
+    loc = EXPR_LOCATION (ref.ptr);
 
-  if (EXPR_HAS_LOCATION (expr))
-    loc = EXPR_LOCATION (expr);
-
   loc = expansion_point_location_if_in_system_header (loc);
 
   char rangestr[2][64];
@@ -1811,7 +1820,7 @@ wrestrict_dom_walker::check_call (gimple *call)
       || (dstwr && !INTEGRAL_TYPE_P (TREE_TYPE (dstwr))))
     return;
 
-  if (check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
+  if (!check_bounds_or_overlap (call, dst, src, dstwr, NULL_TREE))
     return;
 
   /* Avoid diagnosing the call again.  */
@@ -1823,12 +1832,14 @@ wrestrict_dom_walker::check_call (gimple *call)
 /* Attempt to detect and diagnose invalid offset bounds and (except for
    memmove) overlapping copy in a call expression EXPR from SRC to DST
    and DSTSIZE and SRCSIZE bytes, respectively.  Both DSTSIZE and
-   SRCSIZE may be NULL.  Return false when one or the other has been
-   detected and diagnosed, true otherwise.  */
+   SRCSIZE may be NULL.  DO_WARN is false to detect either problem
+   without issue a warning.  Return the OPT_Wxxx constant corresponding
+   to the warning if one has been detected and zero otherwise.  */
 
-bool
+int
 check_bounds_or_overlap (gimple *call, tree dst, tree src, tree dstsize,
-			 tree srcsize, bool bounds_only /* = false */)
+			 tree srcsize, bool bounds_only /* = false */,
+			 bool do_warn /* = true */)
 {
   location_t loc = gimple_nonartificial_location (call);
   loc = expansion_point_location_if_in_system_header (loc);
@@ -1847,11 +1858,12 @@ check_bounds_or_overlap (gimple *call, tree dst, t
   /* Validate offsets first to make sure they are within the bounds
      of the destination object if its size is known, or PTRDIFF_MAX
      otherwise.  */
-  if (maybe_diag_offset_bounds (loc, call, func, strict, dst, dstref)
-      || maybe_diag_offset_bounds (loc, call, func, strict, src, srcref))
+  if (maybe_diag_offset_bounds (loc, call, func, strict, dstref, do_warn)
+      || maybe_diag_offset_bounds (loc, call, func, strict, srcref, do_warn))
     {
-      gimple_set_no_warning (call, true);
-      return false;
+      if (do_warn)
+	gimple_set_no_warning (call, true);
+      return OPT_Warray_bounds;
     }
 
   bool check_overlap
@@ -1861,7 +1873,7 @@ check_bounds_or_overlap (gimple *call, tree dst, t
 	       && DECL_FUNCTION_CODE (func) != BUILT_IN_MEMMOVE_CHK)));
 
   if (!check_overlap)
-    return true;
+    return 0;
 
   if (operand_equal_p (dst, src, 0))
     {
@@ -1875,10 +1887,10 @@ check_bounds_or_overlap (gimple *call, tree dst, t
 		      "%G%qD source argument is the same as destination",
 		      call, func);
 	  gimple_set_no_warning (call, true);
-	  return false;
+	  return OPT_Wrestrict;
 	}
 
-      return true;
+      return 0;
     }
 
   /* Return false when overlap has been detected.  */
@@ -1885,10 +1897,10 @@ check_bounds_or_overlap (gimple *call, tree dst, t
   if (maybe_diag_overlap (loc, call, acs))
     {
       gimple_set_no_warning (call, true);
-      return false;
+      return OPT_Wrestrict;
     }
 
-  return true;
+  return 0;
 }
 
 gimple_opt_pass *
Index: gcc/gimple-ssa-warn-restrict.h
===================================================================
--- gcc/gimple-ssa-warn-restrict.h	(revision 267925)
+++ gcc/gimple-ssa-warn-restrict.h	(working copy)
@@ -20,7 +20,7 @@
 
 #ifndef GIMPLE_SSA_WARN_RESTRICT_H
 
-extern bool check_bounds_or_overlap (gimple *, tree, tree, tree, tree,
-				     bool = false);
+extern int check_bounds_or_overlap (gimple *, tree, tree, tree, tree,
+				    bool = false, bool = true);
 
 #endif /* GIMPLE_SSA_WARN_RESTRICT_H */
Index: gcc/testsuite/c-c++-common/Wrestrict.c
===================================================================
--- gcc/testsuite/c-c++-common/Wrestrict.c	(revision 267925)
+++ gcc/testsuite/c-c++-common/Wrestrict.c	(working copy)
@@ -636,7 +636,7 @@ void test_strcpy_cst (ptrdiff_t i)
   T ("012", a, a + 3);
   /* The following doesn't overlap but it should trigger -Wstringop-overflow
      for reading past the end.  */
-  T ("012", a, a + sizeof a);
+  T ("012", a, a + sizeof a);     /* { dg-warning "\\\[-Wstringop-overflow" "pr81437" { xfail *-*-* } } */
 
   /* The terminating nul written to d[2] overwrites s[0].  */
   T ("0123", a, a + 2);           /* { dg-warning "accessing 3 bytes at offsets 0 and 2 overlaps 1 byte at offset 2" } */
@@ -651,9 +651,9 @@ void test_strcpy_cst (ptrdiff_t i)
   T ("012", a + 2, a);            /* { dg-warning "accessing 4 bytes at offsets 2 and 0 overlaps 2 bytes at offset 2" "strcpy" } */
   T ("012", a + 3, a);            /* { dg-warning "accessing 4 bytes at offsets 3 and 0 overlaps 1 byte at offset 3" "strcpy" } */
   T ("012", a + 4, a);
-  /* The following doesn't overlap but it should trigger -Wstrinop-ovewrflow
+  /* The following doesn't overlap but it triggers -Wstringop-overflow
      for writing past the end.  */
-  T ("012", a + sizeof a, a);
+  T ("012", a + sizeof a, a);     /* { dg-warning "\\\[-Wstringop-overflow" } */
 }
 
 /* Exercise strcpy with constant or known arguments offset by a range.
Index: gcc/testsuite/gcc.dg/Warray-bounds-37.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds-37.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Warray-bounds-37.c	(working copy)
@@ -0,0 +1,47 @@
+/* PR tree-optimization/88800 - Spurious -Werror=array-bounds for non-taken
+   branch
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern void* memmove (void*, const void*, __SIZE_TYPE__);
+
+struct A
+{
+  const char *s;
+  int n;
+};
+
+void f (void*);
+
+struct B
+{
+  char d[5];
+  int n;
+};
+
+__attribute__ ((always_inline)) inline void
+g (struct B *p, struct A a)
+{
+  int i = a.n;
+  if (i <= 5)
+    p->n = i;
+  else {
+    p->n = -1;
+    f (p);
+  }
+
+  if (p->n >= 0)
+    memmove (p->d, a.s, a.n);   /* { dg-bogus "\\\[-Warray-bounds" } */
+}
+
+void h (void)
+{
+  char c[8] = "";
+
+  struct A a;
+  a.s = c;
+  a.n = 8;
+
+  struct B b;
+  g (&b, a);
+}
Index: gcc/testsuite/gcc.dg/builtin-memcpy-2.c
===================================================================
--- gcc/testsuite/gcc.dg/builtin-memcpy-2.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/builtin-memcpy-2.c	(working copy)
@@ -0,0 +1,40 @@
+/* PR tree-optimization/88800 - Spurious -Werror=array-bounds for non-taken
+   branch
+   Verify that out-of-bounds memcpy calls are not folded even when
+   warnings are disabled.
+   { dg-do compile }
+   { dg-options "-O2 -w" } */
+
+extern void* memcpy (void*, const void*, __SIZE_TYPE__);
+
+char a1[1], a2[2], a4[4], a8[8], a16[16], a32[32];
+
+void f1 (const void *p)
+{
+  __builtin_memcpy (a1, p, sizeof a1 * 2);
+}
+
+void f2 (const void *p)
+{
+  __builtin_memcpy (a2, p, sizeof a2 * 2);
+}
+
+void f4 (const void *p)
+{
+  __builtin_memcpy (a4, p, sizeof a4 * 2);
+}
+
+void f8 (const void *p)
+{
+  __builtin_memcpy (a8, p, sizeof a8 * 2);
+}
+
+void f16 (const void *p)
+{
+  __builtin_memcpy (a16, p, sizeof a16 * 2);
+}
+
+void f32 (const void *p)
+{
+  __builtin_memcpy (a32, p, sizeof a32 * 2);
+}
Index: gcc/testsuite/gcc.dg/builtin-memcpy.c
===================================================================
--- gcc/testsuite/gcc.dg/builtin-memcpy.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/builtin-memcpy.c	(working copy)
@@ -0,0 +1,41 @@
+/* PR tree-optimization/88800 - Spurious -Werror=array-bounds for non-taken
+   branch
+   Verify that out-of-bounds memcpy calls are not folded when warnings are
+   enabled (builtin-memcpy-2.c verifies they're not folded with warnings
+   disabled).
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern void* memcpy (void*, const void*, __SIZE_TYPE__);
+
+char a1[1], a2[2], a4[4], a8[8], a16[16], a32[32];
+
+void f1 (const void *p)
+{
+  memcpy (a1, p, sizeof a1 * 2);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void f2 (const void *p)
+{
+  memcpy (a2, p, sizeof a2 * 2);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void f4 (const void *p)
+{
+  memcpy (a4, p, sizeof a4 * 2);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void f8 (const void *p)
+{
+  memcpy (a8, p, sizeof a8 * 2);    /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void f16 (const void *p)
+{
+  memcpy (a16, p, sizeof a16 * 2);  /* { dg-warning "\\\[-Warray-bounds" } */
+}
+
+void f32 (const void *p)
+{
+  memcpy (a32, p, sizeof a32 * 2);  /* { dg-warning "\\\[-Warray-bounds" } */
+}
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 267925)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1742,7 +1742,7 @@ handle_builtin_strcpy (enum built_in_function bcod
 
   if (const strinfo *chksi = olddsi ? olddsi : dsi)
     if (si
-	&& !check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, len))
+	&& check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, len))
       {
 	gimple_set_no_warning (stmt, true);
 	set_no_warning = true;
@@ -2214,7 +2214,7 @@ handle_builtin_stxncpy (built_in_function, gimple_
   else
     srcsize = NULL_TREE;
 
-  if (!check_bounds_or_overlap (stmt, dst, src, dstsize, srcsize))
+  if (check_bounds_or_overlap (stmt, dst, src, dstsize, srcsize))
     {
       gimple_set_no_warning (stmt, true);
       return;
@@ -2512,7 +2512,7 @@ handle_builtin_strcat (enum built_in_function bcod
 
 	tree sptr = si && si->ptr ? si->ptr : src;
 
-	if (!check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
+	if (check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
 	  {
 	    gimple_set_no_warning (stmt, true);
 	    set_no_warning = true;
@@ -2622,7 +2622,7 @@ handle_builtin_strcat (enum built_in_function bcod
 
       tree sptr = si && si->ptr ? si->ptr : src;
 
-      if (!check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
+      if (check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
 	{
 	  gimple_set_no_warning (stmt, true);
 	  set_no_warning = true;

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Richard Biener]

On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
>
> The gimple_fold_builtin_memory_op() function folds calls to memcpy
> and similar to MEM_REF when the size of the copy is a small power
> of 2, but it does so without considering whether the copy might
> write (or read) past the end of one of the objects.  To detect
> these kinds of errors (and help distinguish them from -Westrict)
> the folder calls into the wrestrict pass and lets it diagnose them.
> Unfortunately, that can lead to false positives for even some fairly
> straightforward code that is ultimately found to be unreachable.
> PR 88800 is a report of one such problem.
>
> To avoid these false positives the attached patch adjusts
> the function to avoid issuing -Warray-bounds for out-of-bounds
> calls to memcpy et al.  Instead, the patch disables the folding
> of such invalid calls (and only those).  Those that are not
> eliminated during DCE or other subsequent passes are eventually
> diagnosed by the wrestrict pass.
>
> Since this change required removing the dependency of the detection
> on the warning options (originally done as a micro-optimization to
> avoid spending compile-time cycles on something that wasn't needed)
> the patch also adds tests to verify that code generation is not
> affected as a result of warnings being enabled or disabled.  With
> the patch as is, the invalid memcpy calls end up emitted (currently
> they are folded into equally invalid MEM_REFs).  At some point,
> I'd like us to consider whether they should be replaced with traps
> (possibly under the control of  as has been proposed a number of
> times in the past.  If/when that's done, these tests will need to
> be adjusted to look for traps instead.
>
> Tested on x86_64-linux.

I've said in the past that I feel delaying of folding is wrong.

To understand, the PR is about emitting a warning for out-of-bound
accesses in a dead code region?

If we think delaying/disablign the folding is the way to go the
patch looks OK.

Richard.

>
> Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Martin Sebor]

On 1/15/19 4:07 AM, Richard Biener wrote:
> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
>> and similar to MEM_REF when the size of the copy is a small power
>> of 2, but it does so without considering whether the copy might
>> write (or read) past the end of one of the objects.  To detect
>> these kinds of errors (and help distinguish them from -Westrict)
>> the folder calls into the wrestrict pass and lets it diagnose them.
>> Unfortunately, that can lead to false positives for even some fairly
>> straightforward code that is ultimately found to be unreachable.
>> PR 88800 is a report of one such problem.
>>
>> To avoid these false positives the attached patch adjusts
>> the function to avoid issuing -Warray-bounds for out-of-bounds
>> calls to memcpy et al.  Instead, the patch disables the folding
>> of such invalid calls (and only those).  Those that are not
>> eliminated during DCE or other subsequent passes are eventually
>> diagnosed by the wrestrict pass.
>>
>> Since this change required removing the dependency of the detection
>> on the warning options (originally done as a micro-optimization to
>> avoid spending compile-time cycles on something that wasn't needed)
>> the patch also adds tests to verify that code generation is not
>> affected as a result of warnings being enabled or disabled.  With
>> the patch as is, the invalid memcpy calls end up emitted (currently
>> they are folded into equally invalid MEM_REFs).  At some point,
>> I'd like us to consider whether they should be replaced with traps
>> (possibly under the control of  as has been proposed a number of
>> times in the past.  If/when that's done, these tests will need to
>> be adjusted to look for traps instead.
>>
>> Tested on x86_64-linux.
> 
> I've said in the past that I feel delaying of folding is wrong.
> 
> To understand, the PR is about emitting a warning for out-of-bound
> accesses in a dead code region?

Yes.  I am keeping in my mind your preference of not delaying
the folding of valid code.

> 
> If we think delaying/disablign the folding is the way to go the
> patch looks OK.

I do, at least for now.  I'm taking this as your approval to commit
the patch (please let me know if you didn't mean it that way).

Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Jeff Law]

On 1/15/19 8:21 AM, Martin Sebor wrote:
> On 1/15/19 4:07 AM, Richard Biener wrote:
>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
>>> and similar to MEM_REF when the size of the copy is a small power
>>> of 2, but it does so without considering whether the copy might
>>> write (or read) past the end of one of the objects.  To detect
>>> these kinds of errors (and help distinguish them from -Westrict)
>>> the folder calls into the wrestrict pass and lets it diagnose them.
>>> Unfortunately, that can lead to false positives for even some fairly
>>> straightforward code that is ultimately found to be unreachable.
>>> PR 88800 is a report of one such problem.
>>>
>>> To avoid these false positives the attached patch adjusts
>>> the function to avoid issuing -Warray-bounds for out-of-bounds
>>> calls to memcpy et al.  Instead, the patch disables the folding
>>> of such invalid calls (and only those).  Those that are not
>>> eliminated during DCE or other subsequent passes are eventually
>>> diagnosed by the wrestrict pass.
>>>
>>> Since this change required removing the dependency of the detection
>>> on the warning options (originally done as a micro-optimization to
>>> avoid spending compile-time cycles on something that wasn't needed)
>>> the patch also adds tests to verify that code generation is not
>>> affected as a result of warnings being enabled or disabled.  With
>>> the patch as is, the invalid memcpy calls end up emitted (currently
>>> they are folded into equally invalid MEM_REFs).  At some point,
>>> I'd like us to consider whether they should be replaced with traps
>>> (possibly under the control of  as has been proposed a number of
>>> times in the past.  If/when that's done, these tests will need to
>>> be adjusted to look for traps instead.
>>>
>>> Tested on x86_64-linux.
>>
>> I've said in the past that I feel delaying of folding is wrong.
>>
>> To understand, the PR is about emitting a warning for out-of-bound
>> accesses in a dead code region?
> 
> Yes.  I am keeping in my mind your preference of not delaying
> the folding of valid code.
> 
>>
>> If we think delaying/disablign the folding is the way to go the
>> patch looks OK.
> 
> I do, at least for now.  I'm taking this as your approval to commit
> the patch (please let me know if you didn't mean it that way).
Note we are in stage4, so we're supposed to be addressing regression
bugfixes and documentation issues.

So  I think Richi needs to be explicit about whether or not he wants
this in gcc-9 or if it should defer to gcc-10.

I have no technical objections to the patch and would easily ack it in
stage1 or stage3.

Jeff

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Martin Sebor]

On 1/16/19 6:14 PM, Jeff Law wrote:
> On 1/15/19 8:21 AM, Martin Sebor wrote:
>> On 1/15/19 4:07 AM, Richard Biener wrote:
>>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
>>>> and similar to MEM_REF when the size of the copy is a small power
>>>> of 2, but it does so without considering whether the copy might
>>>> write (or read) past the end of one of the objects.  To detect
>>>> these kinds of errors (and help distinguish them from -Westrict)
>>>> the folder calls into the wrestrict pass and lets it diagnose them.
>>>> Unfortunately, that can lead to false positives for even some fairly
>>>> straightforward code that is ultimately found to be unreachable.
>>>> PR 88800 is a report of one such problem.
>>>>
>>>> To avoid these false positives the attached patch adjusts
>>>> the function to avoid issuing -Warray-bounds for out-of-bounds
>>>> calls to memcpy et al.  Instead, the patch disables the folding
>>>> of such invalid calls (and only those).  Those that are not
>>>> eliminated during DCE or other subsequent passes are eventually
>>>> diagnosed by the wrestrict pass.
>>>>
>>>> Since this change required removing the dependency of the detection
>>>> on the warning options (originally done as a micro-optimization to
>>>> avoid spending compile-time cycles on something that wasn't needed)
>>>> the patch also adds tests to verify that code generation is not
>>>> affected as a result of warnings being enabled or disabled.  With
>>>> the patch as is, the invalid memcpy calls end up emitted (currently
>>>> they are folded into equally invalid MEM_REFs).  At some point,
>>>> I'd like us to consider whether they should be replaced with traps
>>>> (possibly under the control of  as has been proposed a number of
>>>> times in the past.  If/when that's done, these tests will need to
>>>> be adjusted to look for traps instead.
>>>>
>>>> Tested on x86_64-linux.
>>>
>>> I've said in the past that I feel delaying of folding is wrong.
>>>
>>> To understand, the PR is about emitting a warning for out-of-bound
>>> accesses in a dead code region?
>>
>> Yes.  I am keeping in my mind your preference of not delaying
>> the folding of valid code.
>>
>>>
>>> If we think delaying/disablign the folding is the way to go the
>>> patch looks OK.
>>
>> I do, at least for now.  I'm taking this as your approval to commit
>> the patch (please let me know if you didn't mean it that way).
> Note we are in stage4, so we're supposed to be addressing regression
> bugfixes and documentation issues.
> 
> So  I think Richi needs to be explicit about whether or not he wants
> this in gcc-9 or if it should defer to gcc-10.
> 
> I have no technical objections to the patch and would easily ack it in
> stage1 or stage3.

The warning is a regression introduced in GCC 8.  I was just about
to commit the fix so please let me know if I should hold off until
stage 1.

Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Richard Biener]

On Thu, Jan 17, 2019 at 2:51 AM Martin Sebor <msebor@gmail.com> wrote:
>
> On 1/16/19 6:14 PM, Jeff Law wrote:
> > On 1/15/19 8:21 AM, Martin Sebor wrote:
> >> On 1/15/19 4:07 AM, Richard Biener wrote:
> >>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
> >>>>
> >>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
> >>>> and similar to MEM_REF when the size of the copy is a small power
> >>>> of 2, but it does so without considering whether the copy might
> >>>> write (or read) past the end of one of the objects.  To detect
> >>>> these kinds of errors (and help distinguish them from -Westrict)
> >>>> the folder calls into the wrestrict pass and lets it diagnose them.
> >>>> Unfortunately, that can lead to false positives for even some fairly
> >>>> straightforward code that is ultimately found to be unreachable.
> >>>> PR 88800 is a report of one such problem.
> >>>>
> >>>> To avoid these false positives the attached patch adjusts
> >>>> the function to avoid issuing -Warray-bounds for out-of-bounds
> >>>> calls to memcpy et al.  Instead, the patch disables the folding
> >>>> of such invalid calls (and only those).  Those that are not
> >>>> eliminated during DCE or other subsequent passes are eventually
> >>>> diagnosed by the wrestrict pass.
> >>>>
> >>>> Since this change required removing the dependency of the detection
> >>>> on the warning options (originally done as a micro-optimization to
> >>>> avoid spending compile-time cycles on something that wasn't needed)
> >>>> the patch also adds tests to verify that code generation is not
> >>>> affected as a result of warnings being enabled or disabled.  With
> >>>> the patch as is, the invalid memcpy calls end up emitted (currently
> >>>> they are folded into equally invalid MEM_REFs).  At some point,
> >>>> I'd like us to consider whether they should be replaced with traps
> >>>> (possibly under the control of  as has been proposed a number of
> >>>> times in the past.  If/when that's done, these tests will need to
> >>>> be adjusted to look for traps instead.
> >>>>
> >>>> Tested on x86_64-linux.
> >>>
> >>> I've said in the past that I feel delaying of folding is wrong.
> >>>
> >>> To understand, the PR is about emitting a warning for out-of-bound
> >>> accesses in a dead code region?
> >>
> >> Yes.  I am keeping in my mind your preference of not delaying
> >> the folding of valid code.
> >>
> >>>
> >>> If we think delaying/disablign the folding is the way to go the
> >>> patch looks OK.
> >>
> >> I do, at least for now.  I'm taking this as your approval to commit
> >> the patch (please let me know if you didn't mean it that way).
> > Note we are in stage4, so we're supposed to be addressing regression
> > bugfixes and documentation issues.
> >
> > So  I think Richi needs to be explicit about whether or not he wants
> > this in gcc-9 or if it should defer to gcc-10.
> >
> > I have no technical objections to the patch and would easily ack it in
> > stage1 or stage3.
>
> The warning is a regression introduced in GCC 8.  I was just about
> to commit the fix so please let me know if I should hold off until
> stage 1.

As it fixes a regression it is fine now - I also like the cleaning up
of the helper function to work independently of the -W* state.

Richard.

> Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Christophe Lyon]

Hi Martin,


On Thu, 17 Jan 2019 at 02:51, Martin Sebor <msebor@gmail.com> wrote:
>
> On 1/16/19 6:14 PM, Jeff Law wrote:
> > On 1/15/19 8:21 AM, Martin Sebor wrote:
> >> On 1/15/19 4:07 AM, Richard Biener wrote:
> >>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
> >>>>
> >>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
> >>>> and similar to MEM_REF when the size of the copy is a small power
> >>>> of 2, but it does so without considering whether the copy might
> >>>> write (or read) past the end of one of the objects.  To detect
> >>>> these kinds of errors (and help distinguish them from -Westrict)
> >>>> the folder calls into the wrestrict pass and lets it diagnose them.
> >>>> Unfortunately, that can lead to false positives for even some fairly
> >>>> straightforward code that is ultimately found to be unreachable.
> >>>> PR 88800 is a report of one such problem.
> >>>>
> >>>> To avoid these false positives the attached patch adjusts
> >>>> the function to avoid issuing -Warray-bounds for out-of-bounds
> >>>> calls to memcpy et al.  Instead, the patch disables the folding
> >>>> of such invalid calls (and only those).  Those that are not
> >>>> eliminated during DCE or other subsequent passes are eventually
> >>>> diagnosed by the wrestrict pass.
> >>>>
> >>>> Since this change required removing the dependency of the detection
> >>>> on the warning options (originally done as a micro-optimization to
> >>>> avoid spending compile-time cycles on something that wasn't needed)
> >>>> the patch also adds tests to verify that code generation is not
> >>>> affected as a result of warnings being enabled or disabled.  With
> >>>> the patch as is, the invalid memcpy calls end up emitted (currently
> >>>> they are folded into equally invalid MEM_REFs).  At some point,
> >>>> I'd like us to consider whether they should be replaced with traps
> >>>> (possibly under the control of  as has been proposed a number of
> >>>> times in the past.  If/when that's done, these tests will need to
> >>>> be adjusted to look for traps instead.
> >>>>
> >>>> Tested on x86_64-linux.
> >>>
> >>> I've said in the past that I feel delaying of folding is wrong.
> >>>
> >>> To understand, the PR is about emitting a warning for out-of-bound
> >>> accesses in a dead code region?
> >>
> >> Yes.  I am keeping in my mind your preference of not delaying
> >> the folding of valid code.
> >>
> >>>
> >>> If we think delaying/disablign the folding is the way to go the
> >>> patch looks OK.
> >>
> >> I do, at least for now.  I'm taking this as your approval to commit
> >> the patch (please let me know if you didn't mean it that way).
> > Note we are in stage4, so we're supposed to be addressing regression
> > bugfixes and documentation issues.
> >
> > So  I think Richi needs to be explicit about whether or not he wants
> > this in gcc-9 or if it should defer to gcc-10.
> >
> > I have no technical objections to the patch and would easily ack it in
> > stage1 or stage3.
>
> The warning is a regression introduced in GCC 8.  I was just about
> to commit the fix so please let me know if I should hold off until
> stage 1.
>

After your commit (r268037), I'm seeing excess errors on some arm targets:
FAIL: c-c++-common/Wrestrict.c  -Wc++-compat  (test for excess errors)
Excess errors:
/gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy'
accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
bytes at offset [2, 3] [-Wrestrict]
/gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy'
accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
bytes at offset [2, 3] [-Wrestrict]
/gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy'
accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
bytes at offset [2, 3] [-Wrestrict]


This is not true for all arm toolchains, so for instance if you want
to reproduce it, you can build for target arm-eabi and keep default
cpu/fpu/mode.
Or force -march=armv5t when running the test.

To give you an idea, you can look at
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/268043/report-build-info.html
the red cells correspond to the regressions, you can deduce the configure flags.

Christophe

> Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Rainer Orth]

Hi Christophe,

> After your commit (r268037), I'm seeing excess errors on some arm targets:
> FAIL: c-c++-common/Wrestrict.c  -Wc++-compat  (test for excess errors)
> Excess errors:
> /gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]
> /gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]
> /gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]

I'm seeing the same on sparc-sun-solaris2.*, both 32 and 64-bit.
Test results for x86_64-w64-mingw32 and ia64-suse-linux-gnu show the same
failure.

Besides (and probably caused by the same revision), I now get

+XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++14 bug  (test for warnings, line 161)
+XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++17 bug  (test for warnings, line 161)
+XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++98 bug  (test for warnings, line 161)

+XPASS: c-c++-common/Warray-bounds-3.c  -Wc++-compat  bug  (test for warnings, line 161)

which is also seen on ia64-suse-linux-gnu.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Martin Sebor]

On 1/18/19 5:24 AM, Rainer Orth wrote:
> Hi Christophe,
> 
>> After your commit (r268037), I'm seeing excess errors on some arm targets:
>> FAIL: c-c++-common/Wrestrict.c  -Wc++-compat  (test for excess errors)
>> Excess errors:
>> /gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
>> /gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
>> /gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
> 
> I'm seeing the same on sparc-sun-solaris2.*, both 32 and 64-bit.
> Test results for x86_64-w64-mingw32 and ia64-suse-linux-gnu show the same
> failure.
> 
> Besides (and probably caused by the same revision), I now get
> 
> +XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++14 bug  (test for warnings, line 161)
> +XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++17 bug  (test for warnings, line 161)
> +XPASS: c-c++-common/Warray-bounds-3.c  -std=gnu++98 bug  (test for warnings, line 161)
> 
> +XPASS: c-c++-common/Warray-bounds-3.c  -Wc++-compat  bug  (test for warnings, line 161)
> 
> which is also seen on ia64-suse-linux-gnu.

I think this is the same problem as the one on arm.  The bigger patch
I posted should take care of it as well.

Martin

Index: gcc/testsuite/c-c++-common/Warray-bounds-3.c
===================================================================
--- gcc/testsuite/c-c++-common/Warray-bounds-3.c	(revision 268082)
+++ gcc/testsuite/c-c++-common/Warray-bounds-3.c	(working copy)
@@ -158,7 +158,7 @@ void test_memcpy_overflow (char *d, const char *s,
       but known access size is detected.  This works except with small
       sizes that are powers of 2 due to bug .  */
    T (char, 1, arr + SR (DIFF_MAX - 1, DIFF_MAX), s, 1);
-  T (char, 1, arr + SR (DIFF_MAX - 1, DIFF_MAX), s, 2);  /* { 
dg-warning "pointer overflow between offset \\\[\[0-9\]+, \[0-9\]+] and 
size 2 accessing array " "bug " { xfail *-*-* } } */
+  T (char, 1, arr + SR (DIFF_MAX - 1, DIFF_MAX), s, 2);  /* { 
dg-warning "pointer overflow between offset \\\[\[0-9\]+, \[0-9\]+] and 
size 2 accessing array " "bug " { xfail fold_memcpy_2 } } */
    T (char, 1, arr + SR (DIFF_MAX - 2, DIFF_MAX), s, 3);  /* { 
dg-warning "pointer overflow between offset \\\[\[0-9\]+, \[0-9\]+] and 
size 3 accessing array " "memcpy" } */
    T (char, 1, arr + SR (DIFF_MAX - 4, DIFF_MAX), s, 5);  /* { 
dg-warning "pointer overflow between offset \\\[\[0-9\]+, \[0-9\]+] and 
size 5 accessing array " "memcpy" } */
  }

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Martin Sebor]

On 1/18/19 2:35 AM, Christophe Lyon wrote:
> Hi Martin,
> 
> 
> On Thu, 17 Jan 2019 at 02:51, Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 1/16/19 6:14 PM, Jeff Law wrote:
>>> On 1/15/19 8:21 AM, Martin Sebor wrote:
>>>> On 1/15/19 4:07 AM, Richard Biener wrote:
>>>>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>
>>>>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
>>>>>> and similar to MEM_REF when the size of the copy is a small power
>>>>>> of 2, but it does so without considering whether the copy might
>>>>>> write (or read) past the end of one of the objects.  To detect
>>>>>> these kinds of errors (and help distinguish them from -Westrict)
>>>>>> the folder calls into the wrestrict pass and lets it diagnose them.
>>>>>> Unfortunately, that can lead to false positives for even some fairly
>>>>>> straightforward code that is ultimately found to be unreachable.
>>>>>> PR 88800 is a report of one such problem.
>>>>>>
>>>>>> To avoid these false positives the attached patch adjusts
>>>>>> the function to avoid issuing -Warray-bounds for out-of-bounds
>>>>>> calls to memcpy et al.  Instead, the patch disables the folding
>>>>>> of such invalid calls (and only those).  Those that are not
>>>>>> eliminated during DCE or other subsequent passes are eventually
>>>>>> diagnosed by the wrestrict pass.
>>>>>>
>>>>>> Since this change required removing the dependency of the detection
>>>>>> on the warning options (originally done as a micro-optimization to
>>>>>> avoid spending compile-time cycles on something that wasn't needed)
>>>>>> the patch also adds tests to verify that code generation is not
>>>>>> affected as a result of warnings being enabled or disabled.  With
>>>>>> the patch as is, the invalid memcpy calls end up emitted (currently
>>>>>> they are folded into equally invalid MEM_REFs).  At some point,
>>>>>> I'd like us to consider whether they should be replaced with traps
>>>>>> (possibly under the control of  as has been proposed a number of
>>>>>> times in the past.  If/when that's done, these tests will need to
>>>>>> be adjusted to look for traps instead.
>>>>>>
>>>>>> Tested on x86_64-linux.
>>>>>
>>>>> I've said in the past that I feel delaying of folding is wrong.
>>>>>
>>>>> To understand, the PR is about emitting a warning for out-of-bound
>>>>> accesses in a dead code region?
>>>>
>>>> Yes.  I am keeping in my mind your preference of not delaying
>>>> the folding of valid code.
>>>>
>>>>>
>>>>> If we think delaying/disablign the folding is the way to go the
>>>>> patch looks OK.
>>>>
>>>> I do, at least for now.  I'm taking this as your approval to commit
>>>> the patch (please let me know if you didn't mean it that way).
>>> Note we are in stage4, so we're supposed to be addressing regression
>>> bugfixes and documentation issues.
>>>
>>> So  I think Richi needs to be explicit about whether or not he wants
>>> this in gcc-9 or if it should defer to gcc-10.
>>>
>>> I have no technical objections to the patch and would easily ack it in
>>> stage1 or stage3.
>>
>> The warning is a regression introduced in GCC 8.  I was just about
>> to commit the fix so please let me know if I should hold off until
>> stage 1.
>>
> 
> After your commit (r268037), I'm seeing excess errors on some arm targets:
> FAIL: c-c++-common/Wrestrict.c  -Wc++-compat  (test for excess errors)
> Excess errors:
> /gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]
> /gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]
> /gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy'
> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
> bytes at offset [2, 3] [-Wrestrict]
> 
> 
> This is not true for all arm toolchains, so for instance if you want
> to reproduce it, you can build for target arm-eabi and keep default
> cpu/fpu/mode.

The warnings are valid, the test just hardcodes the wrong byte counts
in the xfailed dg-warning directives.  I've fixed the byte counts so
that the test just shows XPASSes.

The other issue here is that the -Wrestrict warning only triggers for
built-ins and whether GCC keeps those around or folds them to MEM_REFs
depends on the target.  On common targets, a memcpy (d, d + 2, 4) call,
for instance, (i.e., one with a small power-of-2 size) is folded to
MEM_REF, so there is no -Wrestrict warning despite the overlap.
Strictly, it's a false negative, but in reality it's not a problem
because GCC gives the MEM_REF copy the same safe semantics as with
memmove, so the overlap is benign.

But on targets that optimize for space by default (like arm-eabi)
the folding doesn't happen, memcpy gets called for the overlapping
regions, and we get the helpful warning.

If there was a way to tell at compile time which target the test is
being compiled for, whether a folding or non-folding one, that would
give us a way to conditionalize the dg-warnings and avoid these pesky
regressions.  I just posted a patch to do that so if it's approved,
these failures should all be resolved.  Ultimately, though, I'd like
to make the warnings detect invalid accesses in MEM_REFs as much as
in built-in calls, so this should be just a temporary stop-gap fix.

https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01111.html

Martin

> Or force -march=armv5t when running the test.
> 
> To give you an idea, you can look at
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/268043/report-build-info.html
> the red cells correspond to the regressions, you can deduce the configure flags.
> 
> Christophe
> 
>> Martin

Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Jeff Law]

On 1/18/19 4:43 PM, Martin Sebor wrote:
> On 1/18/19 2:35 AM, Christophe Lyon wrote:
>> Hi Martin,
>>
>>
>> On Thu, 17 Jan 2019 at 02:51, Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 1/16/19 6:14 PM, Jeff Law wrote:
>>>> On 1/15/19 8:21 AM, Martin Sebor wrote:
>>>>> On 1/15/19 4:07 AM, Richard Biener wrote:
>>>>>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <msebor@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy
>>>>>>> and similar to MEM_REF when the size of the copy is a small power
>>>>>>> of 2, but it does so without considering whether the copy might
>>>>>>> write (or read) past the end of one of the objects.  To detect
>>>>>>> these kinds of errors (and help distinguish them from -Westrict)
>>>>>>> the folder calls into the wrestrict pass and lets it diagnose them.
>>>>>>> Unfortunately, that can lead to false positives for even some fairly
>>>>>>> straightforward code that is ultimately found to be unreachable.
>>>>>>> PR 88800 is a report of one such problem.
>>>>>>>
>>>>>>> To avoid these false positives the attached patch adjusts
>>>>>>> the function to avoid issuing -Warray-bounds for out-of-bounds
>>>>>>> calls to memcpy et al.  Instead, the patch disables the folding
>>>>>>> of such invalid calls (and only those).  Those that are not
>>>>>>> eliminated during DCE or other subsequent passes are eventually
>>>>>>> diagnosed by the wrestrict pass.
>>>>>>>
>>>>>>> Since this change required removing the dependency of the detection
>>>>>>> on the warning options (originally done as a micro-optimization to
>>>>>>> avoid spending compile-time cycles on something that wasn't needed)
>>>>>>> the patch also adds tests to verify that code generation is not
>>>>>>> affected as a result of warnings being enabled or disabled.  With
>>>>>>> the patch as is, the invalid memcpy calls end up emitted (currently
>>>>>>> they are folded into equally invalid MEM_REFs).  At some point,
>>>>>>> I'd like us to consider whether they should be replaced with traps
>>>>>>> (possibly under the control of  as has been proposed a number of
>>>>>>> times in the past.  If/when that's done, these tests will need to
>>>>>>> be adjusted to look for traps instead.
>>>>>>>
>>>>>>> Tested on x86_64-linux.
>>>>>>
>>>>>> I've said in the past that I feel delaying of folding is wrong.
>>>>>>
>>>>>> To understand, the PR is about emitting a warning for out-of-bound
>>>>>> accesses in a dead code region?
>>>>>
>>>>> Yes.  I am keeping in my mind your preference of not delaying
>>>>> the folding of valid code.
>>>>>
>>>>>>
>>>>>> If we think delaying/disablign the folding is the way to go the
>>>>>> patch looks OK.
>>>>>
>>>>> I do, at least for now.  I'm taking this as your approval to commit
>>>>> the patch (please let me know if you didn't mean it that way).
>>>> Note we are in stage4, so we're supposed to be addressing regression
>>>> bugfixes and documentation issues.
>>>>
>>>> So  I think Richi needs to be explicit about whether or not he wants
>>>> this in gcc-9 or if it should defer to gcc-10.
>>>>
>>>> I have no technical objections to the patch and would easily ack it in
>>>> stage1 or stage3.
>>>
>>> The warning is a regression introduced in GCC 8.  I was just about
>>> to commit the fix so please let me know if I should hold off until
>>> stage 1.
>>>
>>
>> After your commit (r268037), I'm seeing excess errors on some arm
>> targets:
>> FAIL: c-c++-common/Wrestrict.c  -Wc++-compat  (test for excess errors)
>> Excess errors:
>> /gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
>> /gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
>> /gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy'
>> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2
>> bytes at offset [2, 3] [-Wrestrict]
>>
>>
>> This is not true for all arm toolchains, so for instance if you want
>> to reproduce it, you can build for target arm-eabi and keep default
>> cpu/fpu/mode.
> 
> The warnings are valid, the test just hardcodes the wrong byte counts
> in the xfailed dg-warning directives.  I've fixed the byte counts so
> that the test just shows XPASSes.
> 
> The other issue here is that the -Wrestrict warning only triggers for
> built-ins and whether GCC keeps those around or folds them to MEM_REFs
> depends on the target.  On common targets, a memcpy (d, d + 2, 4) call,
> for instance, (i.e., one with a small power-of-2 size) is folded to
> MEM_REF, so there is no -Wrestrict warning despite the overlap.
> Strictly, it's a false negative, but in reality it's not a problem
> because GCC gives the MEM_REF copy the same safe semantics as with
> memmove, so the overlap is benign.
> 
> But on targets that optimize for space by default (like arm-eabi)
> the folding doesn't happen, memcpy gets called for the overlapping
> regions, and we get the helpful warning.
> 
> If there was a way to tell at compile time which target the test is
> being compiled for, whether a folding or non-folding one, that would
> give us a way to conditionalize the dg-warnings and avoid these pesky
> regressions.  I just posted a patch to do that so if it's approved,
> these failures should all be resolved.  Ultimately, though, I'd like
> to make the warnings detect invalid accesses in MEM_REFs as much as
> in built-in calls, so this should be just a temporary stop-gap fix.
> 
> https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01111.html
I'm not sure if a way to control or predict this stuff. It's dependent
on the target's capabilities, its MOVE_RATIO defintion as well as any
conditions in its block move patterns.

The only way would be to auto-detect this kind of stuff in the dejagnu
framework by passing in suitable C code and analyzing the result.  Ugh.

Not having good knobs/switches on this stuff is a bit of a design &
implementation wart.

I noticed in another thread you were investigating
-m<whatever>-strategy.  All -m arguments are supposed to be target
specific, so you can't rely on those for generic tests.

jeff