From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45451 invoked by alias); 18 Sep 2018 01:31:01 -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 45106 invoked by uid 89); 18 Sep 2018 01:30:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-9.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,LIKELY_SPAM_BODY,SPF_HELO_PASS,TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=So, at, now=c2, as?= 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; Tue, 18 Sep 2018 01:30:05 +0000 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7A3263AA13; Tue, 18 Sep 2018 01:30:03 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-75.rdu2.redhat.com [10.10.112.75]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7B7302010D04; Tue, 18 Sep 2018 01:30:02 +0000 (UTC) Subject: Re: [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028) To: Martin Sebor , Richard Biener Cc: GCC Patches References: <88de1ee3-6ee4-d8d9-3e57-3a42474a4169@redhat.com> <4e613d61-40ad-f05f-9107-5fd7a5c2fdb6@gmail.com> <6aaa70ca-af7f-2b34-43bc-953a02defe03@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: Date: Tue, 18 Sep 2018 01:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <6aaa70ca-af7f-2b34-43bc-953a02defe03@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00949.txt.bz2 On 8/28/18 6:12 PM, Martin Sebor wrote: >>> Sadly, dstbase is the PARM_DECL for d.  That's where things are going >>> "wrong".  Not sure why you're getting the PARM_DECL in that case.  I'd >>> debug get_addr_base_and_unit_offset to understand what's going on. >>> Essentially you're getting different results of >>> get_addr_base_and_unit_offset in a case where they arguably should be >>> the same. >> >> Probably get_attr_nonstring_decl has the same "mistake" and returns >> the PARM_DECL instead of the SSA name pointer.  So we're comparing >> apples and oranges here. > > Returning the SSA_NAME_VAR from get_attr_nonstring_decl() is > intentional but the function need not (perhaps should not) > also set *REF to it. > >> >> Yeah: >> >> /* If EXPR refers to a character array or pointer declared attribute >>    nonstring return a decl for that array or pointer and set *REF to >>    the referenced enclosing object or pointer.  Otherwise returns >>    null.  */ >> >> tree >> get_attr_nonstring_decl (tree expr, tree *ref) >> { >>   tree decl = expr; >>   if (TREE_CODE (decl) == SSA_NAME) >>     { >>       gimple *def = SSA_NAME_DEF_STMT (decl); >> >>       if (is_gimple_assign (def)) >>         { >>           tree_code code = gimple_assign_rhs_code (def); >>           if (code == ADDR_EXPR >>               || code == COMPONENT_REF >>               || code == VAR_DECL) >>             decl = gimple_assign_rhs1 (def); >>         } >>       else if (tree var = SSA_NAME_VAR (decl)) >>         decl = var; >>     } >> >>   if (TREE_CODE (decl) == ADDR_EXPR) >>     decl = TREE_OPERAND (decl, 0); >> >>   if (ref) >>     *ref = decl; >> >> I see a lot of "magic" here again in the attempt to "propagate" >> a nonstring attribute. > > That's the function's purpose: to look for the attribute.  Is > there a better way to do this? > >> Note >> >> foo (char *p __attribute__(("nonstring"))) >> { >>   p = "bar"; >>   strlen (p); // or whatever is necessary to call get_attr_nonstring_decl >> } >> >> is perfectly valid and p as passed to strlen is _not_ nonstring(?). > > I don't know if you're saying that it should get a warning or > shouldn't.  Right now it doesn't because the strlen() call is > folded before we check for nonstring. > > I could see an argument for diagnosing it but I suspect you > wouldn't like it because it would mean more warning from > the folder.  I could also see an argument against it because, > as you said, it's safe. > > If you take the assignment to p away then a warning is issued, > and that's because p is declared with attribute nonstring. > That's also why get_attr_nonstring_decl looks at SSA_NAME_VAR. > >> I think in your code comparing bases you want to look at the _original_ >> argument to the string function rather than what get_attr_nonstring_decl >> returned as ref. > > I've adjusted get_attr_nonstring_decl() to avoid setting *REF > to SSA_NAME_VAR.  That let me remove the GIMPLE_NOP code from > the patch.  I've also updated the comment above SSA_NAME_VAR > to clarify its purpose per Jeff's comments. > > Attached is an updated revision with these changes. > > Martin > > gcc-87028.diff > > PR tree-optimization/87028 - false positive -Wstringop-truncation strncpy with global variable source string > gcc/ChangeLog: > > PR tree-optimization/87028 > * calls.c (get_attr_nonstring_decl): Avoid setting *REF to > SSA_NAME_VAR. > * gimple-fold.c (gimple_fold_builtin_strncpy): Avoid folding > when statement doesn't belong to a basic block. > * tree.h (SSA_NAME_VAR): Update comment. > * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/87028 > * c-c++-common/Wstringop-truncation.c: Remove xfails. > * gcc.dg/Wstringop-truncation-5.c: New test. > > Index: gcc/calls.c > =================================================================== > --- gcc/calls.c (revision 263928) > +++ gcc/calls.c (working copy) > @@ -1503,6 +1503,7 @@ tree > get_attr_nonstring_decl (tree expr, tree *ref) > { > tree decl = expr; > + tree var = NULL_TREE; > if (TREE_CODE (decl) == SSA_NAME) > { > gimple *def = SSA_NAME_DEF_STMT (decl); > @@ -1515,17 +1516,25 @@ get_attr_nonstring_decl (tree expr, tree *ref) > || code == VAR_DECL) > decl = gimple_assign_rhs1 (def); > } > - else if (tree var = SSA_NAME_VAR (decl)) > - decl = var; > + else > + var = SSA_NAME_VAR (decl); > } > > if (TREE_CODE (decl) == ADDR_EXPR) > decl = TREE_OPERAND (decl, 0); > > + /* To simplify calling code, store the referenced DECL regardless of > + the attribute determined below, but avoid storing the SSA_NAME_VAR > + obtained above (it's not useful for dataflow purposes). */ > if (ref) > *ref = decl; > > - if (TREE_CODE (decl) == ARRAY_REF) > + /* Use the SSA_NAME_VAR that was determined above to see if it's > + declared nonstring. Otherwise drill down into the referenced > + DECL. */ > + if (var) > + decl = var; > + else if (TREE_CODE (decl) == ARRAY_REF) > decl = TREE_OPERAND (decl, 0); > else if (TREE_CODE (decl) == COMPONENT_REF) > decl = TREE_OPERAND (decl, 1); The more I look at this the more I think what we really want to be doing is real propagation of the property either via the alias oracle or a propagation engine. You can't even guarantee that if you've got an SSA_NAME that the value it holds has any relation to its underlying SSA_NAME_VAR -- the value in the SSA_NAME could well have been copied from a some other SSA_NAME with a different underlying SSA_NAME_VAR. I'm not going to insist on it, but I think if we find ourselves extending this again in a way that is really working around lack of propagation of the property then we should go back and fix the propagation problem. > Index: gcc/gimple-fold.c > =================================================================== > --- gcc/gimple-fold.c (revision 263925) > +++ gcc/gimple-fold.c (working copy) > @@ -1702,6 +1702,11 @@ gimple_fold_builtin_strncpy (gimple_stmt_iterator > if (tree_int_cst_lt (ssize, len)) > return false; > > + /* Defer warning (and folding) until the next statement in the basic > + block is reachable. */ > + if (!gimple_bb (stmt)) > + return false; > + > /* Diagnose truncation that leaves the copy unterminated. */ > maybe_diag_stxncpy_trunc (*gsi, src, len); I thought Richi wanted the guard earlier (maybe_fold_stmt) -- it wasn't entirely clear to me if the subsequent comments about needing to fold early where meant to raise issues with guarding earlier or not. Jeff