From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111141 invoked by alias); 30 Oct 2017 21:23:59 -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 111127 invoked by uid 89); 30 Oct 2017 21:23:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy= X-HELO: mail-oi0-f52.google.com Received: from mail-oi0-f52.google.com (HELO mail-oi0-f52.google.com) (209.85.218.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Oct 2017 21:23:56 +0000 Received: by mail-oi0-f52.google.com with SMTP id h6so24456073oia.10 for ; Mon, 30 Oct 2017 14:23:56 -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=Q5WnUoawdKjpHYtCslmJ75ovZE0qHvbB9wW+xTYPAH4=; b=MllyyTp8b23QaS8qHEmeojuc1BsbaLAWDEc9x3jlTgZetFN/89/SyNWV+hbcLBz8bo NfHabb/eLkph1CVRzQGR3msLxuP4qvNM3+AaofSozZRvFDor4L1K90JkcN2VOcDHGSjl Z90mTZGCRyrCeVBdJh+ckXq2sj9NyeFmzHgh2Je409u4xbFsvfKkawDP9CPNbX7nz4qU Ler8omHSQsD+Z/bDnUZ1ejoZJ4f/KHayiPbLwNwRcLucLTyrj0TimmAuUonvKJj545cI yU2+F3xIUwEO/O3re2yhx2kV35SmnlywGCn+s3B6cc5C9TFhQmZzd6xIHN7ju2PuzKVx d8fw== X-Gm-Message-State: AMCzsaUrQ0E6OUSfj81v7uWZOpuziNL+aaDLP/c+8wLT2OFUzIsU6WbJ dayMeH5ufsRIRL+OimDh3GI= X-Google-Smtp-Source: ABhQp+SGVDZAJGLy5a8D70/DL3f/74UZtVyPzLi1mbaTotGEPP97phQipT7vBqAqIxmHa7ptGes8YA== X-Received: by 10.157.1.164 with SMTP id e33mr6225903ote.469.1509398635283; Mon, 30 Oct 2017 14:23:55 -0700 (PDT) Received: from [172.20.33.7] ([67.201.107.102]) by smtp.gmail.com with ESMTPSA id v36sm6156835otv.0.2017.10.30.14.23.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 30 Oct 2017 14:23:54 -0700 (PDT) Subject: Re: [PATCH] enhance -Warray-bounds to detect out-of-bounds offsets (PR 82455) To: Richard Biener Cc: Gcc Patch List , Jeff Law References: <79634da6-bf31-b7f0-15f5-0436fc21a51a@gmail.com> <5FF222AD-B155-434B-9C65-721009D1964E@suse.de> From: Martin Sebor Message-ID: <1064925a-ac64-502e-d0dd-85c27e7432f6@gmail.com> Date: Mon, 30 Oct 2017 21:49: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: <5FF222AD-B155-434B-9C65-721009D1964E@suse.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg02255.txt.bz2 On 10/30/2017 02:56 PM, Richard Biener wrote: > On October 30, 2017 9:13:04 PM GMT+01:00, Martin Sebor wrote: >> On 10/30/2017 01:53 PM, Richard Biener wrote: >>> On October 30, 2017 4:19:25 PM GMT+01:00, 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 forwprop? >>>> >>>>> >>>>> 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. >>>> >>>>> >>>>> 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.) >>> >>> On GIMPLE this is indistinguishable from (short *) (p->a) + 3. >> >> Sure, they're both the same: >> >> pa_3 = &p_2(D)->a; >> _1 = pa_3 + 6; >> >> and that's fine because the implementation of the warning sees and >> uses the byte offset from the beginning of a, so I don't understand >> the problem you are pointing out. Can you clarify what you mean? > > It does not access the array but the underlying object. On GIMPLE it is just an address calculation without constraints. But the computation starts with the subobject and so is only valid within the bounds of the subobject. Or are you saying that GCC emits such GIMPLE expressions for valid code? If so, can you give an example of such code? Or, if that's not it, what exactly is your concern with this enhancement? If it's that it's implemented in forwprop, what would be a better place, e.g., earlier in the optimization phase? If it's something something else, I'd appreciate it if you could explain what. Martin > > Richard. > >> Martin >> >>> >>> GIMPLE is neither C nor C++. >>> >>> Richard. >>> >>>> >>>> Martin >>>> >>>>> >>>>> Richard. >>>>> >>>>>> Jeff, this is the enhancement you were interested in when we spoke >>>>>> last week. >>>>>> >>>>>> Thanks >>>>>> Martin >>>>>> >>>>>> $ cat a.c && gcc -O2 -S -Wall a.c >>>>>> struct A { char a[4]; void (*pf)(void); }; >>>>>> >>>>>> void f (struct A *p) >>>>>> { >>>>>> p->a[5] = 'x'; // existing -Warray-bounds >>>>>> >>>>>> strcpy (p->a + 6, "y"); // enhanced -Warray-bounds >>>>>> } >>>>>> >>>>>> a.c: In function ‘f’: >>>>>> a.c:7:3: warning: offset 6 is out of bounds of ‘char[4]’ >>>> [-Warray-bounds] >>>>>> strcpy (p->a + 6, "y"); >>>>>> ^~~~~~~~~~~~~~~~~~~~~~ >>>>>> a.c:1:17: note: member declared here >>>>>> struct A { char a[4]; void (*pf)(void); }; >>>>>> ^ >>>>>> a.c:5:7: warning: array subscript 5 is above array bounds of >>>> ‘char[4]’ >>>>>> [-Warray-bounds] >>>>>> p->a[5] = 'x'; >>>>>> ~~~~^~~ >>> >