From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94773 invoked by alias); 30 Oct 2017 23:29:16 -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 94758 invoked by uid 89); 30 Oct 2017 23:29:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_1,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-io0-f175.google.com Received: from mail-io0-f175.google.com (HELO mail-io0-f175.google.com) (209.85.223.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Oct 2017 23:29:13 +0000 Received: by mail-io0-f175.google.com with SMTP id n137so31101584iod.6 for ; Mon, 30 Oct 2017 16:29:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=VUYyhz8sovcc5h4MnzB4KSczNR5V6B/1fnmRaKVgMP0=; b=nC/14vEkcsmPfYrKRgmrV8UH/i0dTuWy4TGY8oU0MLB8TuXNnKnx0CgNWzlJ5JqFf8 iZJKqpqRtsf26PPzICVoweVrd8jOHtRZ+eUrqB6R2FDuvKi8FkdQzTEHRHQfijgas0VN 9AE1SwRyPSO0DsoYKVcsIS1JACQApYMbFDhDpWBMCgtnlr20vrbyJ8IhV6ax+klNtwog Vzn+EQUragG1GtlXY4ielnTgE1coPrQwCCq5sMqQ4U+9gPt7vezvi1COAeI6yQi0bFpA CTUgY+8vhSlXDF80ijN2ZRovPVZNRl42tjYD2h7reVqxDnXUhedZjCYcYAlQL3H54xwT YeZw== X-Gm-Message-State: AMCzsaWK3uRoQUibAyjIWvbtUVrYnDJJvczOnGBrRkpDJMHKOui4ghB9 RHMLMdxGn5HWAPoX8bK02r+6A82xPTU= X-Google-Smtp-Source: ABhQp+QkeiSzAsJYARa+OpSTRi8lEALduBvwXYZmZ1h34iy8E6kYqzqEaCWgZOtzlzbwzgDlCRj13A== X-Received: by 10.36.39.214 with SMTP id g205mr592409ita.23.1509406151392; Mon, 30 Oct 2017 16:29:11 -0700 (PDT) Received: from [172.20.1.213] ([67.201.107.102]) by smtp.gmail.com with ESMTPSA id s129sm214610itb.22.2017.10.30.16.29.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 30 Oct 2017 16:29:10 -0700 (PDT) Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455) To: Jeff Law , Richard Biener Cc: Gcc Patch List References: <79634da6-bf31-b7f0-15f5-0436fc21a51a@gmail.com> <928a519a-63fd-4e90-8e8f-fcc829d741a2@redhat.com> From: Martin Sebor Message-ID: <79a05abb-7e50-13b3-c409-a129cd319a82@gmail.com> Date: Mon, 30 Oct 2017 23:30:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <928a519a-63fd-4e90-8e8f-fcc829d741a2@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg02263.txt.bz2 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