[PATCH] enhance -Warray-bounds to handle strings and excessive indices

[Richard Biener]

On Sun, Oct 29, 2017 at 5:15 PM, Martin Sebor <msebor@gmail.com> 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.

Well, it would not handle the case of

a.b[offset-bringing-you-bitoffset-overflow].c[i]

but it would handle (compared to your version of the patch)

a.b[j].c[i]

with i being the unreasonably large offset.  That's beause we
look at the bit offset of a.b[j].c thus the _start_ of the array.

I still find the half-address-space case totally pointless to warn about
(even more to get "precise" here by subtracting the offset of the array).
There's not a single testcase that looks motivating to me (like an
error with some signed->unsigned conversion and then GCC magically
figuring out a range you'd warn for)

The diagnostic wording changes like

@@ -6718,7 +6750,8 @@ check_array_ref (location_t location, tree ref,
bool ignore_off_by_one)
           && tree_int_cst_le (low_sub, low_bound))
         {
           warning_at (location, OPT_Warray_bounds,
-      "array subscript is outside array bounds");
+      "array subscript [%E, %E] is outside array bounds of %qT",
+      low_sub, up_sub, artype);
           TREE_NO_WARNING (ref) = 1;
         }
     }

are ok for trunk.

Thanks,
Richard.


>>   I'll go ahead with the first version
>>>
>>> unless you have a different suggestion.
>
>
> But before I commit the patch as is I want to double-check to make
> sure you don't have further suggestions.
>
> Martin
>
> PS For reference the last version of the patch is here:
>
>   https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01304.html