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 <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
