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

[Jeff Law <law at redhat dot com>]

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