From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35364 invoked by alias); 14 Feb 2018 06:14:14 -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 35354 invoked by uid 89); 14 Feb 2018 06:14:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=pm, by, you=c2, *p?= 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; Wed, 14 Feb 2018 06:14:11 +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 691BF8008D; Wed, 14 Feb 2018 06:14:10 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-16.rdu2.redhat.com [10.10.112.16]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C7D99CB0; Wed, 14 Feb 2018 06:14:09 +0000 (UTC) Subject: Re: [PATCH] correct -Wrestrict handling of arrays of arrays (PR 84095) To: Martin Sebor , Gcc Patch List , Jakub Jelinek References: From: Jeff Law Message-ID: Date: Wed, 14 Feb 2018 06:14:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00811.txt.bz2 On 02/01/2018 04:45 PM, Martin Sebor wrote: > The previous patch didn't resolve all the false positives > in the Linux kernel.  The attached is an update that fixes > the remaining one having to do with multidimensional array > members: > >   struct S { char a[2][4]; }; > >   void f (struct S *p, int i) >   { >     strcpy (p->a[0], "012"); >     strcpy (p->a[i] + 1, p->a[0]);   // false positive here >   } > > In the process of fixing this I also made a couple of minor > restructuring changes to the builtin_memref constructor to > in order to make the code easier to follow: I broke it out > into a couple of helper functions and called those. > > As with the first revision of the patch, this one is also > meant to be applied on top of > >   https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html > > Sorry about the late churn.  Even though I tested the original > implementation with the Linux kernel the bugs were only exposed > non-default configurations that I didn't build. > > Jakub, you had concerns about the code in the constructor > and about interpreting the offsets in the diagnostics. > I tried to address those in the patch.  Please review > the changes and let me know if you have any further comments. > > Thanks > Martin > > On 01/30/2018 04:19 PM, Martin Sebor wrote: >> Testing GCC 8 with recent Linux kernel sources has uncovered >> a bug in the handling of arrays of arrays by the -Wrestrict >> checker where it fails to take references to different array >> elements into consideration, issuing false positives. >> >> The attached patch corrects this mistake. >> >> In addition, to make warnings involving excessive offset bounds >> more meaningful (less confusing), I've made a cosmetic change >> to constrain them to the bounds of the accessed object.  I've >> done this in response to multiple comments indicating that >> the warnings are hard to interpret.  This change is meant to >> be applied on top of the patch for bug 83698 (submitted mainly >> to improve the readability of the offsets): >> >>   https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01488.html >> >> Martin > > > gcc-84095.diff > > > PR middle-end/84095 - false-positive -Wrestrict warnings for memcpy within array > > gcc/ChangeLog: > > PR middle-end/84095 > * gimple-ssa-warn-restrict.c (builtin_memref::extend_offset_range): New. > (builtin_memref::set_base_and_offset): Same. Handle inner references. > (builtin_memref::builtin_memref): Factor out parts into > set_base_and_offset and call it. > > gcc/testsuite/ChangeLog: > > PR middle-end/84095 > * c-c++-common/Warray-bounds-3.c: Adjust text of expected warnings. > * c-c++-common/Wrestrict.c: Same. > * gcc.dg/Wrestrict-6.c: Same. > * gcc.dg/Warray-bounds-27.c: New test. > * gcc.dg/Wrestrict-8.c: New test. > * gcc.dg/Wrestrict-9.c: New test. > * gcc.dg/pr84095.c: New test. > > diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c > index 528eb5b..367e05f 100644 > --- a/gcc/gimple-ssa-warn-restrict.c > +++ b/gcc/gimple-ssa-warn-restrict.c > + else if (gimple_nop_p (stmt)) > + expr = SSA_NAME_VAR (expr); > + else > + { > + base = expr; > + return; > } This looks odd. Can you explain what you're trying to do here? I'm not offhand why you'd ever want to extract SSA_NAME_VAR. In general it's primary use is for dumps and debugging info. I won't quite go so far as to say using it for anything else is wrong, but it's certainly something you ought to explain. The rest looks fairly reasonable. It's a bit hard to follow, but I don't think we should do another round of refactoring at this stage. jeff