On 10/29/2017 10:15 AM, Martin Sebor wrote:
> Ping -- please see my reply below.
>
> On 10/20/2017 09:57 AM, Richard Biener wrote:
>>>>>> get_addr_base_and_unit_offset will return NULL if there's any
>>> variable
>>>>>> component in 'ref'. So as written it seems to be dead code (you
>>>>>> want to pass 'arg'?)
>>>>>
>>>>>
>>>>> Sorry, I'm not sure I understand what you mean. What do you think
>>>>> is dead code? The call to get_addr_base_and_unit_offset() is also
>>>>> made for an array of unspecified bound (up_bound is null) and for
>>>>> an array at the end of a struct. For those the function returns
>>>>> non-null, and for the others (arrays of runtime bound) it returns
>>>>> null. (I passed arg instead of ref but I see no difference in
>>>>> my tests.)
>>>>
>>>> If you pass a.b.c[i] it will return NULL, if you pass a.b.c ('arg')
>>> it will
>>>> return the offset of 'c'. If you pass a.b[j].c it will still return
>>> NULL.
>>>> You could use get_ref_base_and_extent which will return the offset
>>>> of a.b[0].c in this case and sets max_size != size - but you are only
>>>> interested in offset. The disadvantage of get_ref_base_and_extent
>>>> is it returns offset in bits thus if the offset is too large for a
>>> HWI
>>>> you'll instead get offset == 0 and max_size == -1.
>>>>
>>>> Thus I'm saying this is dead code for variable array accesses
>>>> (even for the array you are warning about). Yes, for constant index
>>>> and at-struct-end you'll get sth, but the warning is in VRP because
>>>> of variable indexes.
>>>>
>>>> So I suggest to pass 'arg' and use get_ref_base_and_extent
>>>> for some extra precision (and possible lossage for very very large
>>>> structures).
>>>
>>> Computing bit offsets defeats the out-of-bounds flexible array
>>> index detection because the computation overflows (the function
>>> sets the offset to zero).
>>
>> It shouldn't if you pass arg rather than ref.
>
> I suspect you missed my reply in IRC where I confirmed that this
> approach doesn't work for the reason you yourself mentioned above:
>
>>>> The disadvantage of get_ref_base_and_extent
>>>> is it returns offset in bits thus if the offset is too large
>>>> for a HWI you'll instead get offset == 0 and max_size == -1.
>
> This means that using the function you suggest would either prevent
> detecting the out-of-bounds indices that overflow due to the bit
> offset, thus largely defeating the purpose of the patch (to detect
> excessively large indices), or not printing the value of the out-of
> bounds index in the warning in this case, which would in turn mean
> further changes to the rest of the logic.
I think Richi's point is that it's more important (in his opinion) to
handle the varying objects within an access like a.b.c[i] than to handle
cases that would overflow when converted to bits. Thus he'd prefer to
use get_ref_base_and_extent over get_addr_base_and_unit_offset.
I wonder if a hybrid approach here would work.
ie, use get_ref_base_and_extent per Richi's request. When that returns
an max_size of -1, then call get_addr_base_and_unit_offset and if it
returns that the offset is huge, then warn without any deeper analysis.
Yea, this could trip a false positive if someone makes an array that is
half the address space (give or take), but that's probably not at all
common. Whereas the additional cases handled by get_ref_base_and_extent
are perhaps more useful.
Thoughts?
jeff
