From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75620 invoked by alias); 31 Oct 2017 03:40:25 -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 75606 invoked by uid 89); 31 Oct 2017 03:40:24 -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_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 31 Oct 2017 03:40:23 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B60735AFD9; Tue, 31 Oct 2017 03:40:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B60735AFD9 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=law@redhat.com Received: from localhost.localdomain (ovpn-112-29.phx2.redhat.com [10.3.112.29]) by smtp.corp.redhat.com (Postfix) with ESMTP id 00232600CA; Tue, 31 Oct 2017 03:40:20 +0000 (UTC) Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455) To: Martin Sebor , Richard Biener Cc: Gcc Patch List References: <79634da6-bf31-b7f0-15f5-0436fc21a51a@gmail.com> <928a519a-63fd-4e90-8e8f-fcc829d741a2@redhat.com> <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com> From: Jeff Law Message-ID: Date: Tue, 31 Oct 2017 04:32:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg02278.txt.bz2 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. [ ... ] > > 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