On 03/07/2018 12:37 PM, Jeff Law wrote:
On 02/26/2018 10:32 AM, Martin Sebor wrote:
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;
So is this assignment to offrange[1] really correct?
ISTM that it potentially overflows (relative to MAXOBJSIZE) and that
you'd likely be better off just assigning offrange[1] to MAXOBJSIZE.
Or alternately signal to the callers that we can't really analyze the
memory address and inhibit further analysis of the potential overlap of
the objects.
The offsets are stored in offset_int and there is code to deal
with values outside the [-MAXOBJSIZE, MAXOBJSIZE] range, either
by capping them to the bounds of the array/object being accessed
if it's known, or to the MAXOBJSIZE range. There are a number of
places where offsets with unknown values are being added together
and where their sum might end up exceeding that range (e.g., when
dealing with multiple offsets, each in some range) but as long
as the offsets are only added and not multiplied they should never
overflow offset_int. It would be okay to assign MAXOBJSIZE here
instead of adding it, but there are other places with the same
addition (see the bottom of builtin_memref::extend_offset_range)
so doing it here alone would be inconsistent.
@@ -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;
}
This is probably fairly reasonable. My only concern would be that we
handle multi-dimensional arrays correctly. Do you need to handle
ARRAY_TYPEs with a loop?
Yes, good catch, thanks! It's unrelated to this bug (the ICE) but
it seems simple enough to handle at the same time so I've added
the loop and a test to verify it works as expected.
I do have a more general concern. Given that we walk backwards through
pointer casts and ADDR_EXPRs we potentially lose information. For
example we might walk backwards through a cast from a small array to a
larger array type.
Thus later we use the smaller array type when computing the bounds and
subsequent offset from BASE. This could lead to a missed warning as the
computed offset would be too small.
There are many false negatives here. A lot of those I noticed
during testing are because of limitations in the strlen pass
(I must have raised a dozen bugs to track and, hopefully, fix
in stage 1). There are also deficiencies in the restrict pass
itself. For instance, using builtin_object_size to compute
the size of member arrays leads to many because bos computes
the size of the larger object. It's on my to do list to open
bugs for all these false negatives irrespective of the strlen
limitations as a reminder to add test cases for the former.
I'm inclined to ack after fixing offrange[1] computation noted above as
I don't think the patch makes things any worse. As I noted in a prior
message, I don't see concerns with consistency of BASE that Jakub sees here.
Attached is a revision with the loop. I didn't make any
changes to the offrange[1] computation. If you want me to
change it wholesale for all the uses let me know, but I would
prefer to do it in a separate change, after fixing the ICE.
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.
* gcc.dg/Wrestrict-11.c: New test.
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 258339)
+++ gcc/ChangeLog (working copy)
@@ -1,5 +1,13 @@
2018-03-07 Martin Sebor <mse...@redhat.com>
+ 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.
+
+2018-03-07 Martin Sebor <mse...@redhat.com>
+
PR tree-optimization/84468
* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Consider successor
basic block when looking for nul assignment.
Index: gcc/gimple-ssa-warn-restrict.c
===================================================================
--- gcc/gimple-ssa-warn-restrict.c (revision 258339)
+++ 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,20 @@ 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))
+ basetype = TREE_TYPE (basetype);
+ else
+ while (TREE_CODE (basetype) == ARRAY_TYPE)
+ basetype = TREE_TYPE (basetype);
+
if (RECORD_OR_UNION_TYPE_P (basetype))
return false;
}
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog (revision 258339)
+++ gcc/testsuite/ChangeLog (working copy)
@@ -1,5 +1,10 @@
2018-03-07 Martin Sebor <mse...@redhat.com>
+ PR tree-optimization/84526
+ * gcc.dg/Wrestrict-10.c: New test.
+
+2018-03-07 Martin Sebor <mse...@redhat.com>
+
PR tree-optimization/84468
* g++.dg/warn/Wstringop-truncation-2.C: New test.
* gcc.dg/Wstringop-truncation.c: New test.
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: gcc/testsuite/gcc.dg/Wrestrict-11.c
===================================================================
--- gcc/testsuite/gcc.dg/Wrestrict-11.c (nonexistent)
+++ gcc/testsuite/gcc.dg/Wrestrict-11.c (working copy)
@@ -0,0 +1,205 @@
+/* PR tree-optimization/84526 - ICE in generic_overlap
+ Unrelated to the ICE but rather to PR 84095 that introduced it, verify
+ that calls to strncpy involving multidimensional arrays of structs don't
+ trigger false positive -Wrestrict warnings.
+ { dg-do compile }
+ { dg-options "-O2 -Wrestrict -ftrack-macro-expansion=0" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern char* strcpy (char*, const char*);
+
+struct MemArrays
+{
+ char a1[4];
+ char a2[4][4];
+ char a3[4][4][4];
+} ma1[4], ma2[4][4], ma3[4][4][4];
+
+#define T(dst, src) do { \
+ strcpy (src, "123"); \
+ strcpy (dst, src); \
+ } while (0)
+
+
+void test_ma1_cst (const char *s)
+{
+ T (ma1[0].a1, ma1[0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma1[0].a1, ma1[1].a1);
+ T (ma1[0].a1, ma1[2].a1);
+ T (ma1[0].a1, ma1[3].a1);
+
+ T (ma1[0].a1, ma1[0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma1[1].a1, ma1[0].a1);
+ T (ma1[2].a1, ma1[0].a1);
+ T (ma1[3].a1, ma1[0].a1);
+}
+
+
+void test_ma1_var_cst (const char *s, int i)
+{
+ T (ma1[i].a1, ma1[0].a1);
+ T (ma1[i].a1, ma1[1].a1);
+ T (ma1[i].a1, ma1[2].a1);
+ T (ma1[i].a1, ma1[3].a1);
+
+ T (ma1[0].a1, ma1[i].a1);
+ T (ma1[1].a1, ma1[i].a1);
+ T (ma1[2].a1, ma1[i].a1);
+ T (ma1[3].a1, ma1[i].a1);
+}
+
+
+void test_ma1_var_var (const char *s, int i, int j)
+{
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+ T (ma1[i].a1, ma1[j].a1);
+}
+
+
+void test_ma2_cst (const char *s)
+{
+ T (ma2[0][0].a1, ma2[0][0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma2[0][0].a1, ma2[0][1].a1);
+ T (ma2[0][0].a1, ma2[0][2].a1);
+ T (ma2[0][0].a1, ma2[0][3].a1);
+
+ T (ma2[0][0].a1, ma2[1][0].a1);
+ T (ma2[0][0].a1, ma2[1][1].a1);
+ T (ma2[0][0].a1, ma2[1][2].a1);
+ T (ma2[0][0].a1, ma2[1][3].a1);
+
+ T (ma2[0][0].a1, ma2[2][0].a1);
+ T (ma2[0][0].a1, ma2[2][1].a1);
+ T (ma2[0][0].a1, ma2[2][2].a1);
+ T (ma2[0][0].a1, ma2[2][3].a1);
+
+ T (ma2[0][0].a1, ma2[3][0].a1);
+ T (ma2[0][0].a1, ma2[3][1].a1);
+ T (ma2[0][0].a1, ma2[3][2].a1);
+ T (ma2[0][0].a1, ma2[3][3].a1);
+
+
+ T (ma2[0][1].a1, ma2[0][0].a1);
+ T (ma2[0][1].a1, ma2[0][1].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma2[0][1].a1, ma2[0][2].a1);
+ T (ma2[0][1].a1, ma2[0][3].a1);
+
+ T (ma2[0][1].a1, ma2[1][0].a1);
+ T (ma2[0][1].a1, ma2[1][1].a1);
+ T (ma2[0][1].a1, ma2[1][2].a1);
+ T (ma2[0][1].a1, ma2[1][3].a1);
+
+ T (ma2[0][1].a1, ma2[2][0].a1);
+ T (ma2[0][1].a1, ma2[2][1].a1);
+ T (ma2[0][1].a1, ma2[2][2].a1);
+ T (ma2[0][1].a1, ma2[2][3].a1);
+
+ T (ma2[0][1].a1, ma2[3][0].a1);
+ T (ma2[0][1].a1, ma2[3][1].a1);
+ T (ma2[0][1].a1, ma2[3][2].a1);
+ T (ma2[0][1].a1, ma2[3][3].a1);
+
+
+ T (ma2[0][2].a1, ma2[0][0].a1);
+ T (ma2[0][2].a1, ma2[0][1].a1);
+ T (ma2[0][2].a1, ma2[0][2].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma2[0][2].a1, ma2[0][3].a1);
+
+ T (ma2[0][2].a1, ma2[1][0].a1);
+ T (ma2[0][2].a1, ma2[1][1].a1);
+ T (ma2[0][2].a1, ma2[1][2].a1);
+ T (ma2[0][2].a1, ma2[1][3].a1);
+
+ T (ma2[0][2].a1, ma2[2][0].a1);
+ T (ma2[0][2].a1, ma2[2][1].a1);
+ T (ma2[0][2].a1, ma2[2][2].a1);
+ T (ma2[0][2].a1, ma2[2][3].a1);
+
+ T (ma2[0][2].a1, ma2[3][0].a1);
+ T (ma2[0][2].a1, ma2[3][1].a1);
+ T (ma2[0][2].a1, ma2[3][2].a1);
+ T (ma2[0][2].a1, ma2[3][3].a1);
+
+
+ T (ma2[0][3].a1, ma2[0][0].a1);
+ T (ma2[0][3].a1, ma2[0][1].a1);
+ T (ma2[0][3].a1, ma2[0][2].a1);
+ T (ma2[0][3].a1, ma2[0][3].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+
+ T (ma2[0][3].a1, ma2[1][0].a1);
+ T (ma2[0][3].a1, ma2[1][1].a1);
+ T (ma2[0][3].a1, ma2[1][2].a1);
+ T (ma2[0][3].a1, ma2[1][3].a1);
+
+ T (ma2[0][3].a1, ma2[2][0].a1);
+ T (ma2[0][3].a1, ma2[2][1].a1);
+ T (ma2[0][3].a1, ma2[2][2].a1);
+ T (ma2[0][3].a1, ma2[2][3].a1);
+
+ T (ma2[0][3].a1, ma2[3][0].a1);
+ T (ma2[0][3].a1, ma2[3][1].a1);
+ T (ma2[0][3].a1, ma2[3][2].a1);
+ T (ma2[0][3].a1, ma2[3][3].a1);
+}
+
+
+void test_ma2_var (int i0, int j0, int i1, int j1)
+{
+ T (ma2[i0][j0].a1, ma2[i0][j0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+
+ T (ma2[i0][j0].a1, ma2[i0][j1].a1); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma2[i0][j0].a1, ma2[i1][j1].a1); /* { dg-bogus "\\\[-Wrestrict]" } */
+
+ T (ma2[0][0].a2[i0], ma2[0][0].a2[j0]); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma2[0][i0].a2[0], ma2[0][i1].a2[0]); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma2[i0][0].a2[0], ma2[i1][0].a2[0]); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma2[i0][j0].a2[0], ma2[i1][j1].a2[0]); /* { dg-bogus "\\\[-Wrestrict]" } */
+}
+
+
+void test_p2_var (struct MemArrays **p2, int i0, int j0, int i1, int j1)
+{
+ T (p2[i0][j0].a1, p2[i0][j0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+
+ T (p2[i0][j0].a1, p2[i0][j1].a1);
+ T (p2[i0][j0].a1, p2[i1][j1].a1);
+
+ T (p2[0][0].a2[i0], p2[0][0].a2[j0]);
+ T (p2[0][i0].a2[0], p2[0][i1].a2[0]);
+ T (p2[i0][0].a2[0], p2[i1][0].a2[0]);
+ T (p2[i0][j0].a2[0], p2[i1][j1].a2[0]);
+}
+
+
+void test_ma3_cst (const char *s)
+{
+ T (ma3[0][0][0].a1, ma3[0][0][0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+ T (ma3[0][0][0].a1, ma3[0][0][3].a1);
+
+ T (ma3[0][0][0].a1, ma3[0][1][0].a1);
+ T (ma3[0][0][0].a1, ma3[0][1][3].a1);
+ T (ma3[0][0][0].a1, ma3[1][0][0].a1);
+ T (ma3[0][0][0].a1, ma3[1][0][3].a1);
+ T (ma3[0][0][0].a1, ma3[3][0][3].a1);
+ T (ma3[0][0][0].a1, ma3[3][3][3].a1);
+}
+
+
+void test_ma3_var (const char *s,
+ int i0, int j0, int k0,
+ int i1, int j1, int k1)
+{
+ T (ma3[i0][j0][k0].a1, ma3[i0][j0][k0].a1); /* { dg-warning "\\\[-Wrestrict]" } */
+
+ T (ma3[i0][j0][k0].a1, ma3[i0][j0][k1].a1); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma3[i0][j0][k0].a1, ma3[i0][j1][k1].a1); /* { dg-bogus "\\\[-Wrestrict]" } */
+ T (ma3[i0][j0][k0].a1, ma3[i1][j1][k1].a1); /* { dg-bogus "\\\[-Wrestrict]" } */
+}
