From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65295 invoked by alias); 2 Nov 2017 11:27:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 65285 invoked by uid 89); 2 Nov 2017 11:27:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=suse, SUSE, gmbh, jane X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Nov 2017 11:27:04 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5DE09AB9A; Thu, 2 Nov 2017 11:27:02 +0000 (UTC) Date: Thu, 02 Nov 2017 11:27:00 -0000 From: Richard Biener To: Jeff Law cc: Martin Sebor , Gcc Patch List Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455) In-Reply-To: Message-ID: References: <79634da6-bf31-b7f0-15f5-0436fc21a51a@gmail.com> <928a519a-63fd-4e90-8e8f-fcc829d741a2@redhat.com> <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="-1609908220-276662797-1509622022=:12252" X-SW-Source: 2017-11/txt/msg00071.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609908220-276662797-1509622022=:12252 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Content-length: 5424 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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ---1609908220-276662797-1509622022=:12252--