From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74178 invoked by alias); 29 Aug 2018 00:12:56 -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 74168 invoked by uid 89); 29 Aug 2018 00:12:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-10.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=REF, __SIZE_TYPE__, __size_type__ X-HELO: mail-qt0-f194.google.com Received: from mail-qt0-f194.google.com (HELO mail-qt0-f194.google.com) (209.85.216.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Aug 2018 00:12:53 +0000 Received: by mail-qt0-f194.google.com with SMTP id r37-v6so3903449qtc.0 for ; Tue, 28 Aug 2018 17:12:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to; bh=C/7P93wX4gLK5UTsdQb+qx1OZE53kDk1ADbZjG+4rMM=; b=VgjAkadhGbWAslSiT0Qi49DJjqZPwY31N+baytygr4MmYLP51O31ww+sY9Vx46GJdx +rHG3z1gr1Rg1ixqHe6l93GaCQa0mHcogSJsfJyKZMXn48RWfINLjvxBLXBITSPMAaex A6GI+QaPwxtRsa0fLFVwbJ2UxPOYCdflqsvluo1LUWkDzfTM8xFsz2tSq1IlEpWy4ZkO IPIzUY3C6E8NO2ktEvNqvkhdJ4cke2uEynzf2dXtV+MP1ylzwpyhyYpr4iarO0WIhrqS NgfkxT1IBEmN0jpnFrWppnWH+ZtHY5capQdvNFuwJC/60lmFyvtZbA52iqzu7Ofu5ccj BVKA== Return-Path: Received: from localhost.localdomain (75-166-100-32.hlrn.qwest.net. [75.166.100.32]) by smtp.gmail.com with ESMTPSA id q18-v6sm326313qtk.57.2018.08.28.17.12.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Aug 2018 17:12:49 -0700 (PDT) Subject: Re: [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028) To: Richard Biener , Jeff Law References: <88de1ee3-6ee4-d8d9-3e57-3a42474a4169@redhat.com> <4e613d61-40ad-f05f-9107-5fd7a5c2fdb6@gmail.com> Cc: GCC Patches From: Martin Sebor Message-ID: <6aaa70ca-af7f-2b34-43bc-953a02defe03@gmail.com> Date: Wed, 29 Aug 2018 00:12: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: multipart/mixed; boundary="------------E785D5CD408E187D7859C85F" X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg01818.txt.bz2 This is a multi-part message in MIME format. --------------E785D5CD408E187D7859C85F Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2977 >> 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 --------------E785D5CD408E187D7859C85F Content-Type: text/x-patch; name="gcc-87028.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-87028.diff" Content-length: 7597 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/tree.h =================================================================== --- gcc/tree.h (revision 263925) +++ gcc/tree.h (working copy) @@ -1697,7 +1697,10 @@ extern tree maybe_wrap_with_location (tree, locati : NULL_TREE) /* Returns the variable being referenced. This can be NULL_TREE for - temporaries not associated with any user variable. + temporaries not associated with any user variable. The result + is mainly useful for debugging, diagnostics, or as the target + declaration referenced by an SSA_NAME. Otherwise, because + it has no dataflow information, it should not be used. Once released, this is the only field that can be relied upon. */ #define SSA_NAME_VAR(NODE) \ (SSA_NAME_CHECK (NODE)->ssa_name.var == NULL_TREE \ 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); 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); Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 263925) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1904,8 +1904,6 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi if (TREE_CODE (dstdecl) == ADDR_EXPR) dstdecl = TREE_OPERAND (dstdecl, 0); - tree ref = NULL_TREE; - if (!sidx) { /* If the source is a non-string return early to avoid warning @@ -1914,12 +1912,14 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi tree srcdecl = gimple_call_arg (stmt, 1); if (TREE_CODE (srcdecl) == ADDR_EXPR) srcdecl = TREE_OPERAND (srcdecl, 0); - if (get_attr_nonstring_decl (srcdecl, &ref)) + if (get_attr_nonstring_decl (srcdecl, NULL)) return false; } - /* Likewise, if the destination refers to a an array/pointer declared - nonstring return early. */ + /* Likewise, if the destination refers to an array/pointer declared + nonstring return early. REF will be set to the referenced enclosing + object or pointer either way. */ + tree ref; if (get_attr_nonstring_decl (dstdecl, &ref)) return false; Index: gcc/testsuite/c-c++-common/Wstringop-truncation.c =================================================================== --- gcc/testsuite/c-c++-common/Wstringop-truncation.c (revision 263925) +++ gcc/testsuite/c-c++-common/Wstringop-truncation.c (working copy) @@ -329,9 +329,8 @@ void test_strncpy_array (Dest *pd, int i, const ch of the array to NUL is not diagnosed. */ { /* This might be better written using memcpy() but it's safe so - it probably shouldn't be diagnosed. It currently triggers - a warning because of bug 81704. */ - strncpy (dst7, "0123456", sizeof dst7); /* { dg-bogus "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */ + it isn't diagnosed. See pr81704 and pr87028. */ + strncpy (dst7, "0123456", sizeof dst7); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ dst7[sizeof dst7 - 1] = '\0'; sink (dst7); } @@ -350,7 +349,7 @@ void test_strncpy_array (Dest *pd, int i, const ch } { - strncpy (pd->a5, "01234", sizeof pd->a5); /* { dg-bogus "\\\[-Wstringop-truncation]" "bug 81704" { xfail *-*-* } } */ + strncpy (pd->a5, "01234", sizeof pd->a5); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ pd->a5[sizeof pd->a5 - 1] = '\0'; sink (pd); } Index: gcc/testsuite/gcc.dg/Wstringop-truncation-5.c =================================================================== --- gcc/testsuite/gcc.dg/Wstringop-truncation-5.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wstringop-truncation-5.c (working copy) @@ -0,0 +1,64 @@ +/* PR tree-optimization/87028 - false positive -Wstringop-truncation + strncpy with global variable source string + { dg-do compile } + { dg-options "-O2 -Wstringop-truncation" } */ + +char *strncpy (char *, const char *, __SIZE_TYPE__); + +#define STR "1234567890" + +struct S +{ + char a[5], b[5]; +}; + +const char arr[] = STR; +const char* const ptr = STR; + +const char arr2[][10] = { "123", STR }; + +void test_literal (struct S *s) +{ + strncpy (s->a, STR, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + s->a[sizeof s->a - 1] = '\0'; +} + +void test_global_arr (struct S *s) +{ + strncpy (s->a, arr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + s->a [sizeof s->a - 1] = '\0'; +} + +void test_global_arr2 (struct S *s) +{ + strncpy (s->a, arr2[1], sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + s->a [sizeof s->a - 1] = '\0'; + + strncpy (s->b, arr2[0], sizeof s->a - 1); +} + +void test_global_ptr (struct S *s) +{ + strncpy (s->a, ptr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + s->a [sizeof s->a - 1] = '\0'; +} + +void test_local_arr (struct S *s) +{ + const char arr[] = STR; + strncpy (s->a, arr, sizeof s->a - 1); + s->a [sizeof s->a - 1] = '\0'; +} + +void test_local_ptr (struct S *s) +{ + const char* const ptr = STR; + strncpy (s->a, ptr, sizeof s->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + s->a [sizeof s->a - 1] = '\0'; +} + +void test_compound_literal (struct S *s) +{ + strncpy (s->a, (char[]){ STR }, sizeof s->a - 1); + s->a [sizeof s->a - 1] = '\0'; +} --------------E785D5CD408E187D7859C85F--