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.
> @@ -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?
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.
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.
jeff
