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
