This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fix ICE in generic_overlap (PR 84526)


On 02/23/2018 01:13 PM, Jakub Jelinek wrote:
On Fri, Feb 23, 2018 at 12:57:14PM -0700, Martin Sebor wrote:
+  /* get_inner_reference is not expected to return null.  */
+  gcc_assert (base != NULL);
+
   poly_int64 bytepos = exact_div (bitpos, BITS_PER_UNIT);

-  HOST_WIDE_INT const_off;
-  if (!base || !bytepos.is_constant (&const_off))
-    {
-      base = get_base_address (TREE_OPERAND (expr, 0));
-      return;
-    }
-
+  /* There is no conversion from poly_int64 to offset_int even
+     though the latter is wider, so go through HOST_WIDE_INT.
+     The offset is expected to always be constant.  */
+  HOST_WIDE_INT const_off = bytepos.to_constant ();

The assert is ok, but removing the bytepos.is_constant (&const_off)
is wrong, I'm sure Richard S. can come up with some SVE testcase
where it will not be constant.  If it is not constant, you can handle
it like var_off (which as I said on IRC or in the PR also seems to be
incorrect, because if the base is not a decl the variable offset could be
negative).

That's what I did at first.  I took it out because it sounded
to me like you were saying it was a hypothetical possibility,
not something that would ever happen in practice, and because
I have no way of testing that code.  I have put it back in
the attached update.

Since you added Richard: can you please explain how to convert
a poly64_int to offset_int without converting it to HOST_WIDE_INT,
or if it can't be done, why not?  It's cumbersome and error-prone
to have to go through HWI, and doing so defeats the main goal of
using offset_int in the gimple-ssa-warn-restrict pass.  Should
the pass (and others like it) be changed to store offsets and
sizes in some flavor of poly_int instead of offset_int?

   offrange[0] += const_off;
   offrange[1] += const_off;

@@ -923,7 +923,11 @@ builtin_access::generic_overlap ()
       /* There's no way to distinguish an access to the same member
 	 of a structure from one to two distinct members of the same
 	 structure.  Give up to avoid excessive false positives.  */
-      tree basetype = TREE_TYPE (TREE_TYPE (dstref->base));
+      tree basetype = TREE_TYPE (dstref->base);
+      if (POINTER_TYPE_P (basetype)
+	  || TREE_CODE (basetype) == ARRAY_TYPE)
+	basetype = TREE_TYPE (basetype);

This doesn't address any of my concerns that it is completely random
what {dst,src}ref->base is, apples and oranges; sometimes it is a pointer
(e.g. the argument of the function), sometimes the ADDR_EXPR operand,
sometimes the base of the reference, sometimes again address (if the
base of the reference is MEM_REF).  By the lack of consistency in what
it is, just deciding on its type whether you take TREE_TYPE or
TREE_TYPE (TREE_TYPE ()) of it also gives useless result.  You could e.g
call the memcpy etc. function with ADDR_EXPR of a VAR_DECL that has pointer
type, then if dstref->base is that VAR_DECL, POINTER_TYPE_P (basetype)
would be true.

I think I understand what you're saying but this block is only
used for string functions (not for memcpy), and only as a stopgap
to avoid false positives.  Being limited to (a subset of) string
functions the case I think you're concerned about, namely calling
strcpy with a pointer to a pointer, shouldn't come up in valid
code.  It's not bullet-proof but I don't think there is
a fundamental problem, either with this patch or with the one
that introduced it.  The fundamental problem is that MEM_REF
can be ambiguous and that's what this code is trying to combat.
See the example I gave here:
https://gcc.gnu.org/ml/gcc/2018-02/msg00136.html

If you think I'm missing something please put together a small
test case to help me see the problem.  I'm not nearly as
comfortable thinking in terms of the internal representation
to visualize all the possibilities here.

Martin
PR tree-optimization/84526 - ICE in generic_overlap at gcc/gimple-ssa-warn-restrict.c:927 since r257860

gcc/ChangeLog:

	PR tree-optimization/84526
	* gimple-ssa-warn-restrict.c (builtin_memref::set_base_and_offset):
	Remove dead code.
	(builtin_access::generic_overlap): Be prepared to handle non-array
	base objects.

gcc/testsuite/ChangeLog:

	PR tree-optimization/84526
	* gcc.dg/Wrestrict-10.c: New test.

Index: gcc/gimple-ssa-warn-restrict.c
===================================================================
--- gcc/gimple-ssa-warn-restrict.c	(revision 257933)
+++ gcc/gimple-ssa-warn-restrict.c	(working copy)
@@ -43,6 +43,8 @@
 #include "cfgloop.h"
 #include "intl.h"
 
+location_t gloc;
+
 namespace {
 
 const pass_data pass_data_wrestrict = {
@@ -409,18 +411,24 @@ builtin_memref::set_base_and_offset (tree expr)
   base = get_inner_reference (expr, &bitsize, &bitpos, &var_off,
 			      &mode, &sign, &reverse, &vol);
 
+  /* get_inner_reference is not expected to return null.  */
+  gcc_assert (base != NULL);
+
   poly_int64 bytepos = exact_div (bitpos, BITS_PER_UNIT);
 
+  /* There is no conversion from poly_int64 to offset_int even
+     though the latter is wider, so go through HOST_WIDE_INT.
+     The offset should be constant but be prepared for it not
+     to be just in case.  */
   HOST_WIDE_INT const_off;
-  if (!base || !bytepos.is_constant (&const_off))
+  if (bytepos.is_constant (&const_off))
     {
-      base = get_base_address (TREE_OPERAND (expr, 0));
-      return;
+      offrange[0] += const_off;
+      offrange[1] += const_off;
     }
+  else
+    offrange[1] += maxobjsize;
 
-  offrange[0] += const_off;
-  offrange[1] += const_off;
-
   if (var_off)
     {
       if (TREE_CODE (var_off) == INTEGER_CST)
@@ -918,12 +926,18 @@ builtin_access::generic_overlap ()
   if (!overlap_certain)
     {
       if (!dstref->strbounded_p && !depends_p)
+	/* Memcpy only considers certain overlap.  */
 	return false;
 
       /* There's no way to distinguish an access to the same member
 	 of a structure from one to two distinct members of the same
 	 structure.  Give up to avoid excessive false positives.  */
-      tree basetype = TREE_TYPE (TREE_TYPE (dstref->base));
+      tree basetype = TREE_TYPE (dstref->base);
+
+      if (POINTER_TYPE_P (basetype)
+	  || TREE_CODE (basetype) == ARRAY_TYPE)
+	basetype = TREE_TYPE (basetype);
+
       if (RECORD_OR_UNION_TYPE_P (basetype))
 	return false;
     }
@@ -1845,6 +1859,7 @@ check_bounds_or_overlap (gcall *call, tree dst, tr
       loc = *pbloc;
 
   loc = expansion_point_location_if_in_system_header (loc);
+  gloc = loc;
 
   tree func = gimple_call_fndecl (call);
 
Index: gcc/testsuite/gcc.dg/Wrestrict-10.c
===================================================================
--- gcc/testsuite/gcc.dg/Wrestrict-10.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wrestrict-10.c	(working copy)
@@ -0,0 +1,121 @@
+/* PR tree-optimization/84526 - ICE in generic_overlap
+   { dg-do compile }
+   { dg-options "-O2 -Wrestrict" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void* restrict, const void* restrict, size_t);
+extern char* strcat (char* restrict, const char* restrict);
+extern char* strcpy (char* restrict, const char* restrict);
+extern char* strncat (char* restrict, const char* restrict, size_t);
+extern char* strncpy (char* restrict, const char* restrict, size_t);
+
+struct
+{
+  char a[1];
+} b;
+
+int i;
+size_t n;
+
+void __attribute__ ((noclone, noinline))
+test_arr_memcpy_1 (void)
+{
+  memcpy (&b.a[i], b.a, n);
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_memcpy_2 (void)
+{
+  memcpy (b.a, &b.a[i], n);
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strcat_1 (void)
+{
+  strcat (&b.a[i], b.a);            /* { dg-warning "\\\[-Wrestrict" } */
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strcat_2 (void)
+{
+  /* This probably deserves a warning.  */
+  strcpy (b.a, &b.a[i]);
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strncat_1 (void)
+{
+  strncat (&b.a[i], b.a, n);        /* { dg-warning "\\\[-Wrestrict" } */
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strncat_2 (void)
+{
+  strncat (b.a, &b.a[i], n);        /* { dg-warning "\\\[-Wrestrict" } */
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strcpy_1 (void)
+{
+  strcpy (&b.a[i], b.a);
+}
+
+void __attribute__ ((noclone, noinline))
+test_arr_strcpy_2 (void)
+{
+  strcpy (b.a, &b.a[i]);
+}
+
+
+struct S {
+  int a;
+  char b[10];
+} d;
+
+void __attribute__ ((noclone, noinline))
+test_obj_memcpy_1 (void)
+{
+  memcpy (d.b, (char *) &d, n);
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_memcpy_2 (void)
+{
+  memcpy ((char *) &d, d.b, n);
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_strcpy_1 (void)
+{
+  strcpy (d.b, (char *) &d);
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_strcpy_2 (void)
+{
+  strcpy ((char *) &d, d.b);
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_strncat_1 (void)
+{
+  strncat (d.b, (char *) &d, n);    /* { dg-warning "\\\[-Wrestrict" } */
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_strncat_2 (void)
+{
+  strncat ((char *) &d, d.b, n);    /* { dg-warning "\\\[-Wrestrict" } */
+}
+
+void __attribute__ ((noclone, noinline))
+test_obj_strncpy_1 (void)
+{
+  strncpy (d.b, (char *) &d, n);
+}
+
+void test_obj_strncpy_2 (void)
+{
+  strncpy ((char *) &d, d.b, n);
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]