From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6182 invoked by alias); 15 Feb 2018 17:48:00 -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 6167 invoked by uid 89); 15 Feb 2018 17:47:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:4097 X-HELO: mail-oi0-f42.google.com Received: from mail-oi0-f42.google.com (HELO mail-oi0-f42.google.com) (209.85.218.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Feb 2018 17:47:57 +0000 Received: by mail-oi0-f42.google.com with SMTP id x21so379631oie.13 for ; Thu, 15 Feb 2018 09:47:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=SWo6zZKk48La9msVCuA3JjQbX9vwTSIUm2FS8MCiSNY=; b=Cg+hC6/L/aKDB77TU7acDUaBc+5nTReJPWy8muMHq5C54DFzz1Q4O3VEIa+5etPoCi FOVt8vIzZl+w55wg3dXQ8L6axciw9grD6VPDajmtrJwoJHfM1oLadwz7+FLRZYdHkGm8 NsQpJMMsXUVKI+ZtyULIw1dS/MzyvNaPSkUqymcJTJbYzG6S4KsclNyPFFq5R07WYsvz zP20koKSRwxttwlxcARUlBuTjuLKMka08s8SaXsU9NbYQE9uoiwhKSXIWq5mPsHWViIv txPk6JPn/Kj8Ovh3hxE09hM1KFT3n7x1NCweNy5Z99z6RkgdGDN8CG8UIEgH+Nn+admK a/AA== X-Gm-Message-State: APf1xPCvSMnz/TI//kC/G6MK2aB031KZxRQns/bCmcBNpTEkssNPXlf/ FCYH5Uv/1IOk5cNe1mjeRVU= X-Google-Smtp-Source: AH8x225sfmUNT8JD5/Aim6y2zeaIbkn3kXj/U7wElsxd2n7SlSMd6Pbw6ljF1wuxQSarWwEanwr8+g== X-Received: by 10.202.221.11 with SMTP id u11mr2427287oig.42.1518716875757; Thu, 15 Feb 2018 09:47:55 -0800 (PST) Received: from localhost.localdomain (75-171-228-29.hlrn.qwest.net. [75.171.228.29]) by smtp.gmail.com with ESMTPSA id h11sm8466460otj.37.2018.02.15.09.47.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Feb 2018 09:47:54 -0800 (PST) Subject: Re: [PATCH] correct -Wrestrict handling of arrays of arrays (PR 84095) To: Jeff Law , Gcc Patch List , Jakub Jelinek References: From: Martin Sebor Message-ID: Date: Thu, 15 Feb 2018 17:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00912.txt.bz2 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? Martin