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

[Martin Sebor <msebor@gmail.com>]

On 10/30/2017 03:48 PM, Jeff Law wrote:
> 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.

I put it in forwprop only because that was the last stage where
there's still enough context before the POINTER_PLUS_EXPR is
folded into MEM_REF to tell an offset from the beginning of
a subobject from the one from the beginning of the bigger object
of which the subobject is a member.  I certainly don't mind moving
it somewhere else more appropriate if this isn't ideal, just as
long it doesn't cripple the detection (e.g., as long as we still
have range information).

>>> 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.

I of course don't want to break anything.  I didn't see any fallout
in my testing and I normally test all the front ends, including Ada,
but let me check to make sure I tested it this time (I had made some
temporary changes to my build script and may have disabled it.)  Let
me double check it after I get back from my trip.

Martin