From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122909 invoked by alias); 14 Feb 2018 05:36:45 -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 122858 invoked by uid 89); 14 Feb 2018 05:36:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=Hx-languages-length:2636 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 05:36:41 +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 EFFE1C0587C0; Wed, 14 Feb 2018 05:36:38 +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 239D060561; Wed, 14 Feb 2018 05:36:37 +0000 (UTC) Subject: Re: [PATCH] make -Wrestrict for strcat more meaningful (PR 83698) To: Martin Sebor , Jakub Jelinek Cc: Gcc Patch List References: <20180116213254.GO2063@tucnak> <3fe66f05-1c06-ada0-b27b-b4a017677851@gmail.com> From: Jeff Law Message-ID: Date: Wed, 14 Feb 2018 05:36: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: <3fe66f05-1c06-ada0-b27b-b4a017677851@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00809.txt.bz2 On 01/16/2018 05:35 PM, Martin Sebor wrote: > On 01/16/2018 02:32 PM, Jakub Jelinek wrote: >> On Tue, Jan 16, 2018 at 01:36:26PM -0700, Martin Sebor wrote: >>> --- gcc/gimple-ssa-warn-restrict.c    (revision 256752) >>> +++ gcc/gimple-ssa-warn-restrict.c    (working copy) >>> @@ -384,6 +384,12 @@ builtin_memref::builtin_memref (tree expr, tree si >>>        base = SSA_NAME_VAR (base); >>>        } >>> >>> +  if (DECL_P (base) && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE) >>> +    { >>> +      if (offrange[0] < 0 && offrange[1] > 0) >>> +    offrange[0] = 0; >>> +    } >> >> Why the 2 nested ifs? > > No particular reason.  There may have been more code in there > that I ended up removing.  Or a comment.  I can remove the > extra braces when the patch is approved. > >> >>> @@ -1079,14 +1085,35 @@ builtin_access::strcat_overlap () >>>      return false; >>> >>>    /* When strcat overlap is certain it is always a single byte: >>> -     the terminatinn NUL, regardless of offsets and sizes.  When >>> +     the terminating NUL, regardless of offsets and sizes.  When >>>       overlap is only possible its range is [0, 1].  */ >>>    acs.ovlsiz[0] = dstref->sizrange[0] == dstref->sizrange[1] ? 1 : 0; >>>    acs.ovlsiz[1] = 1; >>> -  acs.ovloff[0] = (dstref->sizrange[0] + >>> dstref->offrange[0]).to_shwi (); >>> -  acs.ovloff[1] = (dstref->sizrange[1] + >>> dstref->offrange[1]).to_shwi (); >> >> You use to_shwi many times in the patch, do the callers or something >> earlier >> in this function guarantee that you aren't throwing away any bits (unlike >> tree_to_shwi, to_shwi method doesn't ICE, just throws away upper bits). >> Especially when you perform additions like here, even if both >> wide_ints fit >> into a shwi, the result might not. > > No, I'm not sure.  In fact, it wouldn't surprise me if it did > happen.  It doesn't cause false positives or negatives but it > can make the offsets less than meaningful in cases where they > are within valid bounds.  There are also cases where they are > meaningless to begin with and there is little the pass can do > about that. I was kind of expecting an update to try and address some of these issues. Though after re-reading your response the consequence of throwing away bits here is just the diagnostic is not as precise as it could be, right? ie, it doesn't change when we issue a diagnostic, just the contents of the diagnostic. I filed this into my gcc9 bucket because it doesn't fix a regression, but it appears that a regression fix does depend on this stuff to some degree (84095). So I'll try to take a look at this shortly so that we can unblock 84095. Jeff