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).
>
>> 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.
The intent of the code is that BASE is a pointer to a memory location
that can be compared to another BASE for the purposes of trying to
determine if there's an overlap between two arguments.
Note that BASE may have a different type than what actually appeared in
the call. Martin's code is presented with the actual argument, but then
tries to walk backwards to refine that value. In particular it might
look through pointer typecasts or pointer arithmetic. The latter
results in a memory address plus an offset.
It may also look at an ADDR_EXPR and strip away its various _REF nodes.
The result in this case is similar to pointer arithmetic in that we will
have a memory address plus an offset.
Note that the memory address computed in here and stored into BASE may
differ in its type from what is actually passed to the function for
which we are checking for invalid argument overlap. In fact, I think
The key here is boil arguments down to a memory location plus an
optional offset, which in turn allows us to compare them against each
other for overlap. In some ways it's like what we do in alias.c for RTL
(which isn't necessary a shining example of good code).
I don't think the lack of consistency is really a major issue inside
builtin_memref::set_base_and_offset -- as long as clients use the
information just for warnings and are aware that the underlying types
may differ from what appeared in the call itself.
Removal of the undesirable base = get_base_address (TREE_OPERAND (expr,
0)) definitely helps clarify IMHO.
I'll have more comments as a follow-up to one of Martin's messages.
Jeff
