On 03/07/2018 03:52 PM, Martin Sebor wrote:
> 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.
OK. Thanks for explaining.
>
>>> @@ -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.
Agreed. Thanks for fixing this as well. Agreed it seems simple enough
to go ahead and handle now.
>>
>> 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.
Understood. I should have been clearer that I don't think we
necessarily have to address the false negatives right now. Just that
this is a potential source of them.
>
>> 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
>
>
> gcc-84526.diff
>
>
> 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.
OK. I think Richard S. had some follow-up issues that didn't affect
correctness/behavior. Those clean ups are pre-approved.
Given we've seen a few instances pop up in Fedora, I'm going to go ahead
and commit now. At the least it'll stop duplicates in our BZ and if
Jakub is spinning a new Fedora release over the weekend, it'd get picked
up for Fedora as well.
Jeff
