From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3124 invoked by alias); 16 Feb 2018 23:39:55 -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 3109 invoked by uid 89); 16 Feb 2018 23:39:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: 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= 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; Fri, 16 Feb 2018 23:39:52 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B39FA81DE2; Fri, 16 Feb 2018 23:39:51 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-67.rdu2.redhat.com [10.10.112.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 99E7B5D6A2; Fri, 16 Feb 2018 23:39:50 +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: <549771ff-19bd-0a29-81bb-7c0e2c15ed23@redhat.com> Date: Fri, 16 Feb 2018 23:39: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/msg01047.txt.bz2 On 02/15/2018 10:47 AM, Martin Sebor wrote: > On 02/13/2018 11:14 PM, Jeff Law wrote: >> 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. > > It appears to be dead code.  Nothing in the GCC test suite hits > this code.  It's most likely a vestige of an approach I tried > that didn't work and that I ended up doing differently and forgot > to remove.  I'll remove it before committing. > >> 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. > > Is the patch good to commit then with the unused code above > removed? Yes. Again, sorry for the delays. jeff