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

[Jeff Law <law at redhat dot com>]

On 02/23/2018 02:46 PM, Martin Sebor wrote:
>> 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.
I think at least part of the issue here was that previously you had this
code in set_base_and_offset:

 HOST_WIDE_INT const_off;
  if (!base || !bytepos.is_constant (&const_off))
    {
      base = get_base_address (TREE_OPERAND (expr, 0));
      return;
    }

That is stripping away part of EXPR in a way I don't think was safe.
With this gone in the most recent patches I'm a lot less concerned.

Jeff