That attached revision updates the pass to directly convert
the poly64_int into offset_int using the API suggested by
Richard (thanks again). As discussed below, I've made no
other changes.
Jakub, if you still have concerns that the false positive
suppression logic isn't sufficiently effective please post
a test case (or open a new bug). I will look into it when
I get a chance.
Until then, I'd like to get the ICE fix committed. Please
confirm that the attached patch is good to go in.
Martin
On 02/23/2018 02:46 PM, Martin Sebor wrote:
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 257963)
+++ gcc/gimple-ssa-warn-restrict.c (working copy)
@@ -396,6 +396,9 @@ builtin_memref::set_base_and_offset (tree expr)
if (TREE_CODE (expr) == ADDR_EXPR)
expr = TREE_OPERAND (expr, 0);
+ /* Stash the reference for offset validation. */
+ ref = expr;
+
poly_int64 bitsize, bitpos;
tree var_off;
machine_mode mode;
@@ -409,23 +412,33 @@ 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);
- HOST_WIDE_INT const_off;
- if (!base || !bytepos.is_constant (&const_off))
+ /* Convert the poly_int64 offset to to offset_int. The offset
+ should be constant but be prepared for it not to be just in
+ case. */
+ offset_int cstoff;
+ if (bytepos.is_constant (&cstoff))
{
- base = get_base_address (TREE_OPERAND (expr, 0));
- return;
+ offrange[0] += cstoff;
+ offrange[1] += cstoff;
+
+ /* Besides the reference saved above, also stash the offset
+ for validation. */
+ if (TREE_CODE (expr) == COMPONENT_REF)
+ refoff = cstoff;
}
+ else
+ offrange[1] += maxobjsize;
- offrange[0] += const_off;
- offrange[1] += const_off;
-
if (var_off)
{
if (TREE_CODE (var_off) == INTEGER_CST)
{
- offset_int cstoff = wi::to_offset (var_off);
+ cstoff = wi::to_offset (var_off);
offrange[0] += cstoff;
offrange[1] += cstoff;
}
@@ -433,13 +446,6 @@ builtin_memref::set_base_and_offset (tree expr)
offrange[1] += maxobjsize;
}
- /* Stash the reference for offset validation. */
- ref = expr;
-
- /* Also stash the constant offset for offset validation. */
- if (TREE_CODE (expr) == COMPONENT_REF)
- refoff = const_off;
-
if (TREE_CODE (base) == MEM_REF)
{
tree memrefoff = TREE_OPERAND (base, 1);
@@ -918,12 +924,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;
}
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);
+}
