Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455)

[Jeff Law <law@redhat.com>]

On 10/30/2017 09:19 AM, Martin Sebor wrote:
> On 10/30/2017 05:45 AM, Richard Biener wrote:
>> On Sun, 29 Oct 2017, Martin Sebor wrote:
>>
>>> In my work on -Wrestrict, to issue meaningful warnings, I found
>>> it important to detect both out of bounds array indices as well
>>> as offsets in calls to restrict-qualified functions like strcpy.
>>> GCC already detects some of these cases but my tests for
>>> the enhanced warning exposed a few gaps.
>>>
>>> The attached patch enhances -Warray-bounds to detect more instances
>>> out-of-bounds indices and offsets to member arrays and non-array
>>> members.  For example, it detects the out-of-bounds offset in the
>>> call to strcpy below.
>>>
>>> The patch is meant to be applied on top posted here but not yet
>>> committed:
>>>    https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01304.html
>>>
>>> Richard, since this also touches tree-vrp.c I look for your comments.
>>
>> You fail to tell what you are changing and why - I have to reverse
>> engineer this from the patch which a) isn't easy in this case, b) feels
>> like a waste of time.  Esp. since the patch does many things.
>>
>> My first question is why do you add a warning from forwprop?  It
>> _feels_ like you're trying to warn about arbitrary out-of-bound
>> addresses at the point they are folded to MEM_REFs.  And it looks
>> like you're warning about pointer arithmetic like &p->a + 6.
>> That doesn't look correct to me.  Pointer arithmetic in GIMPLE
>> is not restricted to operate within fields that are appearantly
>> accessed here - the only restriction is with respect to the
>> whole underlying pointed-to-object.
>>
>> By doing the warning from forwprop you'll run into all such cases
>> introduced by GCC itself during quite late optimization passes.
> 
> I haven't run into any such cases.  What would a more appropriate
> place to detect out-of-bounds offsets?  I'm having a hard time
> distinguishing what is appropriate and what isn't.  For instance,
> if it's okay to detect some out of bounds offsets/indices in vrp
> why is it wrong to do a better job of it in forwpropI think part of the problem is there isn't a well defined place to do
this kind of warning.  I suspect it's currently in VRP simply because
that is where we had range information in the past.  It's still the
location with the most accurate range information.

In a world where we have an embedded context sensitive range analysis
engine, we should *really* look at pulling the out of bounds array
warnings out of any optimization pass an have a distinct pass to deal
with them.

I guess in the immediate term the question I would ask Martin is what is
it about forwprop that makes it interesting?  Is it because of the
lowering issues we touched on last week?  If so I wonder if we could
recreate an array form from a MEM_REF for the purposes of optimization.
Or if we could just handle MEM_REFs better within the existing warning.


> 
>>
>> You're trying to re-do __builtin_object_size even when that wasn't
>> used.
> 
> That's not the quite my intent, although it is close.
Wouldn't we be better off improving _b_o_s?

> 
>>
>> So it looks like you're on the wrong track.  Yes,
>>
>>   strcpy (p->a + 6, "y");
>>
>> _may_ be "invalid" C (I'm not even sure about that!) but it
>> is certainly not invalid GIMPLE.
> 
> Adding (or subtracting) an integer to/from a pointer to an array
> is defined in both C and C++ only if the result points to an element
> of the array or just past the last element of the array.  Otherwise
> it's undefined. (A non-array object is considered an array of one
> for this purpose.)
I think Richi's argument is that gimple allows things that are not
necessarily allowed by the C/C++ standard.  For example we support
virtual origins from Ada, which internally would look something like
invalid C code

OTOH, we currently have code in tree-vrp.c which warns if we compute the
address of an out of bounds array index which is very C/C++ centric.

jeff