This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455)
- From: Richard Biener <rguenther at suse dot de>
- To: Jeff Law <law at redhat dot com>
- Cc: Martin Sebor <msebor at gmail dot com>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 2 Nov 2017 12:27:02 +0100 (CET)
- Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455)
- Authentication-results: sourceware.org; auth=none
- References: <b63d021f-ceca-e26f-848a-40182de004c9@gmail.com> <alpine.LSU.2.20.1710301238080.8202@zhemvz.fhfr.qr> <79634da6-bf31-b7f0-15f5-0436fc21a51a@gmail.com> <928a519a-63fd-4e90-8e8f-fcc829d741a2@redhat.com> <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com> <a770a2a7-f548-7d32-eaee-7358f2c4cea9@redhat.com>
On Mon, 30 Oct 2017, Jeff Law wrote:
> On 10/30/2017 05:29 PM, Martin Sebor wrote:
> > 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).
> Understood.
Well, it's a long-standing issue with how we do these kind of
warnings, likewise for _b_o_s which also "can't stand" component-refs
to be folded into the MEM_REF offset.
I've said in the past that _b_o_s relying on component-refs to stay
and for them to be constrained the same way they are in C is bogus.
We've added an early _b_o_s pass to mitigate that "issue" somewhat.
Now you're trying to "solve" the same issue as _b_o_s -- in the
end it looks like the warning could well reside in that pass
rather than in forwprop.
Richard.
>
> [ ... ]
>
> >
> > 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.
> No worries. Hopefully by the time you're back I'll have something
> publishable on the ripping apart tree-vrp front and we can prototype the
> effectiveness of doing this kind of stuff outside tree-vrp.c
>
> We should also revisit Aldy's work from last year which started the
> whole effort around fixing how we deal with out out of bounds index
> testing. He had a version which ran outside tree-vrp.c but used the
> same basic structure and queried range data for the index. I've got a
> copy here we can poke at.
>
> jeff
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)