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