From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73799 invoked by alias); 29 Nov 2018 23:43:34 -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 73790 invoked by uid 89); 29 Nov 2018 23:43:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Low, eliminated, gimplify, calls.c X-HELO: mail-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Nov 2018 23:43:28 +0000 Received: by mail-qt1-f193.google.com with SMTP id r14so4103348qtp.1 for ; Thu, 29 Nov 2018 15:43:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=tjkTNejqP/IjDLEB4LMeP5AIfg6kPQlIcwB2EgGUpJI=; b=cOQ9NAmaay4igwnr/ghCdvSydnDj14Y+sOLRzKMb+aqz6DiwKEoyAa5u/L12DM10g3 5S+L5u2Z6uk+oQt+hADi/5liRbi4is6VQ2kvjFwn913hgt4T5iAfg3J0YRS3yzZ+knP8 wexko2Ss/S+4KqrS1k89hnfMIHUB+7ktQa9qPmPoqgtFqjgnxG+sANmPqsP204Xtfreq hzKCgnT66tjuLwfWhjchhLV+qHUQlcNp0GjO/nEzvHUtcTLMbxVuh30jE4+u9ZDcD5Nn P8yhOObNNiUrv55V+LHMKDmmlti6mgdtt77EkPItuUnt3XoL9FRelhvTSaZ5En5l/KVR 53qg== Return-Path: Received: from localhost.localdomain (97-118-99-160.hlrn.qwest.net. [97.118.99.160]) by smtp.gmail.com with ESMTPSA id k132sm1641919qke.36.2018.11.29.15.43.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 29 Nov 2018 15:43:25 -0800 (PST) Subject: Re: [PING #4][PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028) To: Jeff Law , Richard Biener Cc: GCC Patches References: <88de1ee3-6ee4-d8d9-3e57-3a42474a4169@redhat.com> <27235a6d-6f2c-1f2d-d456-d4cd9b941894@redhat.com> <23ea3d13-d9c5-1b02-f01c-d2a0e11f3a10@redhat.com> <9e3f6d62-47b9-b80f-b8ac-5711628579c5@redhat.com> <09ce3b57-33a3-86ae-1308-39fd02f25228@gmail.com> <1437ae83-c0c2-e18f-0dc8-92717c2fdcfe@gmail.com> <8a346afb-382f-cac9-a2b7-7107ef678dee@redhat.com> From: Martin Sebor Message-ID: Date: Thu, 29 Nov 2018 23:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <8a346afb-382f-cac9-a2b7-7107ef678dee@redhat.com> Content-Type: multipart/mixed; boundary="------------7F63AB1D6C78216DDD2FFB89" X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg02518.txt.bz2 This is a multi-part message in MIME format. --------------7F63AB1D6C78216DDD2FFB89 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-length: 2920 On 11/29/18 4:07 PM, Jeff Law wrote: > On 11/29/18 1:34 PM, Martin Sebor wrote: >> On 11/16/2018 02:07 AM, Richard Biener wrote: >>> On Fri, Nov 16, 2018 at 4:12 AM Martin Sebor wrote: >>>> >>>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html >>>> >>>> Please let me know if there is something I need to change here >>>> to make the fix acceptable or if I should stop trying. >>> >>> I have one more comment about >>> >>> +  /* Defer warning (and folding) until the next statement in the basic >>> +     block is reachable.  */ >>> +  if (!gimple_bb (stmt)) >>> +    return false; >>> + >>> >>> it's not about the next statement in the basic-block being "reachable" >>> (even w/o a CFG you can use gsi_next()) but rather that the next >>> stmt isn't yet gimplified and thus not inserted into the gimple sequence, >>> right? >> >> No, it's about the current statement not being associated with >> a basic block yet when the warning code runs for the first time >> (during gimplify_expr), and so gsi_next() returning null. >> >>> You apply this to gimple_fold_builtin_strncpy but I'd rather >>> see us not sprinkling this over gimple-fold.c but instead do this >>> in gimplify.c:maybe_fold_stmt, delaying folding until say lowering. >>> >>> See the attached (untested). >> >> I would also prefer this solution.  I had tested it (in response >> to you first mentioning it back in September) and it causes quite >> a bit of fallout in tests that look for the folding to take place >> very early.  See the end of my reply here: >> >>   https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01248.html >> >> But I'm willing to do the test suite cleanup if you think it's >> suitable for GCC 9.  (If you're thinking GCC 10 please let me >> know now.) > The fallout on existing tests is minimal. What's more concerning is > that it doesn't actually pass the new test from Martin's original > submission. We get bogus warnings. > > At least part of the problem is weakness in maybe_diag_stxncpy_trunc. > It can't handle something like this: > > test_literal (char * d, struct S * s) > { > strncpy (d, "1234567890", 3); > _1 = d + 3; > *_1 = 0; > } > > > Note the pointer arithmetic between the strncpy and storing the NUL > terminator. Right. I'm less concerned about this case because it involves a literal that's obviously longer than the destination but it would be nice if the suppression worked here as well in case the literal comes from macro expansion. It will require another tweak. But the test from my patch passes with the changes to calls.c from my patch, so that's an improvement. I have done the test suite cleanup in the attached patch. It was indeed minimal -- not sure why I saw so many failures with my initial approach. I can submit a patch to handle the literal case above as a followup unless you would prefer it done at the same time. Martin --------------7F63AB1D6C78216DDD2FFB89 Content-Type: text/x-patch; name="gcc-87028.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-87028.diff" Content-length: 9164 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. * gcc/gimple-low.c (lower_stmt): Delay foldin built-ins. * gimplify (maybe_fold_stmt): Avoid folding statements that don'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. * gcc.dg/strcmpopt_1.c: Adjust. * gcc.dg/tree-ssa/pr79697.c: Same. Index: gcc/calls.c =================================================================== --- gcc/calls.c (revision 266637) +++ 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-low.c =================================================================== --- gcc/gimple-low.c (revision 266637) +++ gcc/gimple-low.c (working copy) @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-low.h" #include "predict.h" #include "gimple-predict.h" +#include "gimple-fold.h" /* The differences between High GIMPLE and Low GIMPLE are the following: @@ -378,6 +379,12 @@ lower_stmt (gimple_stmt_iterator *gsi, struct lowe gsi_next (gsi); return; } + + /* We delay folding of built calls from gimplification to + here so the IL is in consistent state for the diagnostic + machineries job. */ + if (gimple_call_builtin_p (stmt)) + fold_stmt (gsi); } break; Index: gcc/gimplify.c =================================================================== --- gcc/gimplify.c (revision 266637) +++ gcc/gimplify.c (working copy) @@ -3192,6 +3192,10 @@ maybe_fold_stmt (gimple_stmt_iterator *gsi) return false; else if ((ctx->region_type & ORT_HOST_TEAMS) == ORT_HOST_TEAMS) return false; + /* Delay folding of builtins until the IL is in consistent state + so the diagnostic machinery can do a better job. */ + if (gimple_call_builtin_p (gsi_stmt (*gsi))) + return false; return fold_stmt (gsi); } Index: gcc/testsuite/c-c++-common/Wstringop-truncation.c =================================================================== --- gcc/testsuite/c-c++-common/Wstringop-truncation.c (revision 266637) +++ 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 shouldn't be diagnosed. */ + 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'; +} Index: gcc/testsuite/gcc.dg/fold-bcopy.c =================================================================== --- gcc/testsuite/gcc.dg/fold-bcopy.c (revision 266637) +++ gcc/testsuite/gcc.dg/fold-bcopy.c (working copy) @@ -1,6 +1,6 @@ /* PR tree-optimization/80933 - redundant bzero/bcopy calls not eliminated { dg-do compile } - { dg-options "-O0 -Wall -fdump-tree-gimple" } */ + { dg-options "-O1 -Wall -fdump-tree-lower" } */ void f0 (void *dst, const void *src, unsigned n) { @@ -46,9 +46,9 @@ void f6 (void *p) /* Verify that calls to bcmp, bcopy, and bzero have all been removed and one of each replaced with memcmp, memmove, and memset, respectively. The remaining three should be eliminated. - { dg-final { scan-tree-dump-not "bcmp|bcopy|bzero" "gimple" } } - { dg-final { scan-tree-dump-times "memcmp|memmove|memset" 3 "gimple" } } + { dg-final { scan-tree-dump-not "bcmp|bcopy|bzero" "lower" } } + { dg-final { scan-tree-dump-times "memcmp|memmove|memset" 3 "lower" } } Verify that the bcopy to memmove transformation correctly transposed the source and destination pointer arguments. - { dg-final { scan-tree-dump-times "memmove \\(dst, src" 1 "gimple" } } */ + { dg-final { scan-tree-dump-times "memmove \\(dst, src" 1 "lower" } } */ Index: gcc/testsuite/gcc.dg/strcmpopt_1.c =================================================================== --- gcc/testsuite/gcc.dg/strcmpopt_1.c (revision 266637) +++ gcc/testsuite/gcc.dg/strcmpopt_1.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do run } */ -/* { dg-options "-fdump-tree-gimple" } */ +/* { dg-options "-fdump-tree-lower" } */ #include #include @@ -25,4 +25,4 @@ int main () return 0; } -/* { dg-final { scan-tree-dump-times "strcmp \\(" 2 "gimple" } } */ +/* { dg-final { scan-tree-dump-times "strcmp \\(" 2 "lower" } } */ Index: gcc/testsuite/gcc.dg/tree-ssa/pr79697.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/pr79697.c (revision 266637) +++ gcc/testsuite/gcc.dg/tree-ssa/pr79697.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fdump-tree-gimple -fdump-tree-cddce-details -fdump-tree-optimized" } */ +/* { dg-options "-O2 -fdump-tree-lower -fdump-tree-cddce-details -fdump-tree-optimized" } */ void f(void) { @@ -18,4 +18,4 @@ void h(void) /* { dg-final { scan-tree-dump "Deleting : __builtin_strdup" "cddce1" } } */ /* { dg-final { scan-tree-dump "Deleting : __builtin_strndup" "cddce1" } } */ -/* { dg-final { scan-tree-dump "__builtin_malloc" "gimple" } } */ +/* { dg-final { scan-tree-dump "__builtin_malloc" "lower" } } */ --------------7F63AB1D6C78216DDD2FFB89--