From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20004 invoked by alias); 1 Mar 2018 21:17:11 -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 19992 invoked by uid 89); 1 Mar 2018 21:17:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.9 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= X-HELO: mail-ot0-f196.google.com Received: from mail-ot0-f196.google.com (HELO mail-ot0-f196.google.com) (74.125.82.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Mar 2018 21:17:08 +0000 Received: by mail-ot0-f196.google.com with SMTP id y11so6964199otg.0 for ; Thu, 01 Mar 2018 13:17:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to; bh=4BPOlDFvWP5nnaf7FQN/0P3BwCJ/Ohtd7siH6Qct750=; b=EMxDeJsEOhFrSGOwh/RcaBW35zfxsRbZzx495OsR2lB6U7bSrMHkGQYUiz6DDKlfY5 /8wXpJBsZE8UNo6wwqkfIpaOSTDqNmKWLhozO2nUK0LG0afbQlmRiup1U9siA1TnB28s kK2Mqnj9NlDOOzXwfSNJ17ft1gSBuobhX8+uKGUZYa2iVos4bqnGxqX8Di3ykztfdFXb ivoIndDcQcBelrfIdSUY7FncpUdGWekWPl+H3o5OpjvnVDc0Q0GcBsKhEqWWUxFDIZYA gBo2GreO899+lb36DPT5UDx+MTWe2JFI3zAUzRDnz9PJ8MPDf+VX8PEsQaD9t8c5PlPi w6mA== X-Gm-Message-State: APf1xPDQNDGN3D3ej9Q6ZPY3QeqFtC2axlU8EsxhRr6N1tAoCcYU8kQ/ CUK3CeENBCRRLGodlB1EJ1iEtQ== X-Google-Smtp-Source: AG47ELu8u7ybeh8z0tiinEsiVHmpRtcsCf8E6ClhiKlP7tNuMMEO1VV/bHWa997egBEzDY6pwWjrrA== X-Received: by 10.157.47.104 with SMTP id h95mr2307564otb.171.1519939026289; Thu, 01 Mar 2018 13:17:06 -0800 (PST) Received: from localhost.localdomain (71-218-18-146.hlrn.qwest.net. [71.218.18.146]) by smtp.gmail.com with ESMTPSA id u137sm1063747oia.49.2018.03.01.13.17.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 01 Mar 2018 13:17:05 -0800 (PST) From: Martin Sebor Subject: Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468) To: Jeff Law , Gcc Patch List References: <7116c7c3-43f3-0784-a05b-f68253cd7e5f@gmail.com> Message-ID: Date: Thu, 01 Mar 2018 21:17: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="------------E8CA9F02FC586DDCAF58BA61" X-IsSubscribed: yes X-SW-Source: 2018-03/txt/msg00060.txt.bz2 This is a multi-part message in MIME format. --------------E8CA9F02FC586DDCAF58BA61 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 4182 On 02/27/2018 06:50 PM, Jeff Law wrote: > On 02/26/2018 05:47 PM, Martin Sebor wrote: >> On 02/26/2018 12:13 PM, Jeff Law wrote: >>> On 02/24/2018 05:11 PM, Martin Sebor wrote: >>>> Attached is an updated patch with a fix for a bad assumption >>>> exposed by building the linux kernel. >>>> >>>> On 02/19/2018 07:50 PM, Martin Sebor wrote: >>>>> PR 84468 points out a false positive in -Wstringop-truncation >>>>> in code like this: >>>>> >>>>> struct A { char a[4]; }; >>>>> >>>>> void f (struct A *p, const struct A *q) >>>>> { >>>>> if (p->a) >>>>> strncpy (p->a, q->a, sizeof p->a - 1); // warning here >>>>> >>>>> p->a[3] = '\0'; >>>>> } >>>>> >>>>> The warning is due to the code checking only the same basic block >>>>> as the one with the strncpy call for an assignment to the destination >>>>> to avoid it, but failing to check the successor basic block if there >>>>> is no subsequent statement in the current block. (Eliminating >>>>> the conditional is being tracked in PR 21474.) >>>>> >>>>> The attached test case adds logic to avoid this false positive. >>>>> I don't know under what circumstances there could be more than >>>>> one successor block here so I don't handle that case. >>> So this is feeling more and more like we need to go back to the ideas >>> behind checking the virtual operand chains. >>> >>> The patch as-written does not properly handle the case where BB has >>> multiple outgoing edges. For gcc-8 you could probably get away with >>> checking that you have precisely one outgoing edge without EDGE_ABNORMAL >>> set in its flags in addition to the checks you're already doing. >>> >>> But again, it's feeling more and more like the right fix is to go back >>> and walk the virtual operands. >> >> I intentionally kept the patch as simple as possible to minimize >> risk at this late stage. >> >> Attached is a more robust version that handles multiple outgoing >> edges and avoids those with the EDGE_ABNORMAL bit set. Retested >> on x86_64 and with the Linux kernel. >> >> Enhancing this code to handle more complex cases is on my to-do >> list for stage 1 (e.g., to handle bug 84561 where MEM_REF defeats >> the detection of the nul assignment). > I don't think handling multiple outgoing edges is advisable here. To do > that you have to start thinking about post-dominator analysis at which > point you're better off walking the memory web via VUSE/VDEFs. > > Just verify the block has a single outgoing edge and that the edge is > not marked with EDGE_ABNORMAL. Don't bother with the recursive call. > Assuming you get a suitable block, then look inside. I read you last reply as asking me to handle multiple edges. The original patch handled just one edge and didn't bother with EDGE_ABNORMAL because I wanted to keep it simple. But it sounds like you want me to go back to the first version and just add a check for EDGE_ABNORMAL. The attached patch does that (plus it skips over any non-debug statements in the successor block). Besides the usual I retested it with the Linux kernel. There are just three instances of the warning in my default configuration but I know there are a lot more in more extensive builds. > I glanced over the tests and I didn't see any that would benefit from > handling multiple edges or the recursion (every one of the dg-bogus > markers should be immediately transferring control to the null > termination statement AFAICT). I've added some more test cases involving C++ exceptions (and found one false positive(*)) but I don't have a test to exercise blocks with multiple outgoing edges. For future reference, how would I create one? Martin [*] From bug 84624: char d[3]; void f (); void g (const char *s) { try { f (); } catch (...) { __builtin_strncpy (d, s, sizeof d); // bogus warning } d[sizeof d - 1] = 0; // because of this } The warning sees the following and triggers because the strncpy call is followed by __cxa_end_catch(). [count: 0]: : _1 = __builtin_eh_pointer (1); __cxa_begin_catch (_1); __builtin_strncpy (&d, s_7(D), 3); __cxa_end_catch (); goto ; [100.00%] --------------E8CA9F02FC586DDCAF58BA61 Content-Type: text/x-patch; name="gcc-84468.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-84468.diff" Content-length: 10377 Index: gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C (nonexistent) +++ gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C (working copy) @@ -0,0 +1,164 @@ +// PR tree-optimization/84468 - bogus -Wstringop-truncation despite +// assignment after conditional strncpy +// Compile with -g to verify the warning deals properly with debug +// statements. +// { dg-do compile } +// { dg-options "-O2 -Wstringop-truncation -g" } + +extern "C" char* strncpy (char*, const char*, __SIZE_TYPE__); + +char d[3]; + +void g (); + +void fnowarn1 (const char *s) +{ + // Update dummy but never actually use it so it's eliminated + // but causes debugging statements to be emitted for each + // modification. + int dummy = 0; + + try + { + g (); + strncpy (d, s, sizeof d); // { dg-bogus "\\\[-Wstringop-truncation]" } + ++dummy; + } + catch (...) + { + ++dummy; + d[0] = 0; + } + + ++dummy; + d[sizeof d - 1] = 0; +} + +void fnowarn2 (const char *s) +{ + int dummy = 0; + + try + { + g (); + strncpy (d, s, sizeof d); + ++dummy; + } + catch (...) + { + ++dummy; + return; + } + + ++dummy; + d[sizeof d - 1] = 0; +} + +void fnowarn3 (const char *s) +{ + int dummy = 0; + + try + { + g (); + strncpy (d, s, sizeof d); + ++dummy; + try + { + ++dummy; + d[sizeof d - 1] = 0; + g (); + } + catch (...) + { + ++dummy; + } + } + catch (...) + { + ++dummy; + return; + } + + ++dummy; + d[sizeof d - 1] = 0; +} + +void fnowarn4 (const char *s) +{ + int dummy = 0; + + try + { + g (); + } + catch (...) + { + strncpy (d, s, sizeof d); // { dg-bogus "\\\[-Wstringop-truncation]" "bug 84468" { xfail *-*-*} } + ++dummy; + } + + ++dummy; + d[sizeof d - 1] = 0; +} + +void fwarn1 (const char *s) +{ + int dummy = 0; + + try + { + ++dummy; + g (); + ++dummy; + strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" } + ++dummy; + } + catch (...) + { + ++dummy; + } + + ++dummy; +} + +void fwarn2 (const char *s) +{ + int dummy = 0; + + try + { + ++dummy; + strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" } + ++dummy; + g (); + ++dummy; + } + catch (...) + { + ++dummy; + } + + ++dummy; +} + +void fwarn3 (const char *s) +{ + int dummy = 0; + + try + { + ++dummy; + g (); + ++dummy; + strncpy (d, s, sizeof d); // { dg-warning "\\\[-Wstringop-truncation]" } + ++dummy; + } + catch (...) + { + ++dummy; + d[0] = 0; + } + + ++dummy; +} Index: gcc/testsuite/gcc.dg/Wstringop-truncation-2.c =================================================================== --- gcc/testsuite/gcc.dg/Wstringop-truncation-2.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wstringop-truncation-2.c (working copy) @@ -0,0 +1,126 @@ +/* PR tree-optimization/84468 - bogus -Wstringop-truncation despite + assignment after conditional strncpy + { dg-do compile } + { dg-options "-O2 -Wstringop-truncation -g" } */ + +extern char* strncpy (char*, const char*, __SIZE_TYPE__); + +char a[4]; + +void f1 (char *s) +{ + int i = 0; + + if (s[0] == '0') + { + i += 1; + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + } + else + i += 2; + + a[sizeof a - 1] = 0; +} + +void f2 (char *s) +{ + int i = 0; + + if (s[0] == '0') + { + i += 1; + if (s[1] == '1') + { + i += 2; + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + } + else + i += 3; + } + else + i += 4; + + a[sizeof a - 1] = 0; +} + +void f3 (char *s) +{ + int i = 0; + + if (s[0] == '0') + { + i += 1; + if (s[1] == '1') + { + i += 2; + if (s[2] == '2') + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + else + i += 3; + } + else + i += 4; + } + else + i += 5; + + a[sizeof a - 1] = 0; +} + +void f4 (char *s) +{ + int i = 0; + + if (s[0] == '0') + { + i += 1; + if (s[1] == '1') + { + i += 2; + if (s[2] == '2') + { + i += 3; + if (s[3] == '3') + strncpy (a, s, sizeof a); /* { dg-bogus "\\\[-Wstringop-truncation]" } */ + else + i += 4; + } + else + i += 5; + } + else + i += 6; + } + else + i += 7; + + a[sizeof a - 1] = 0; +} + +void f4_warn (char *s) +{ + int i = 0; + + if (s[0] == '0') + { + i += 1; + if (s[1] == '1') + { + i += 2; + if (s[2] == '2') + { + i += 3; + if (s[3] == '3') + strncpy (a, s, sizeof a); /* { dg-warning "\\\[-Wstringop-truncation]" } */ + else + i += 4; + } + else + i += 5; + } + else + i += 6; + } + else + i += 7; +} Index: gcc/testsuite/gcc.dg/Wstringop-truncation.c =================================================================== --- gcc/testsuite/gcc.dg/Wstringop-truncation.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wstringop-truncation.c (working copy) @@ -0,0 +1,131 @@ +/* PR tree-optimization/84468 - Inconsistent -Wstringop-truncation warnings + with -O2 + { dg-do compile } + { dg-options "-O2 -Wstringop-truncation -ftrack-macro-expansion=0 -g" } */ + +#define strncpy __builtin_strncpy + +struct A +{ + char a[4]; +}; + +void no_pred_succ_lit (struct A *p) +{ + /* The following is folded early on, before the strncpy statement + has a basic block. Verify that the case is handled gracefully + (i.e., there's no assumption that the statement does have + a basic block). */ + strncpy (p->a, "1234", sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */ +} + +/* Verify a strncpy call in a basic block with no predecessor or + successor. */ +void no_pred_succ (struct A *p, const struct A *q) +{ + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */ +} + + +/* Verify a strncpy call in a basic block with no successor. */ +void no_succ (struct A *p, const struct A *q) +{ + if (q->a) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */ +} + +/* Verify a strncpy call in a basic block with nul assignment in + a successor block. */ +void succ (struct A *p, const struct A *q) +{ + /* Verify that the assignment suppresses the warning for the conditional + strcnpy call. The conditional should be folded to true since the + address of an array can never be null (see bug 84470). */ + if (q->a) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + + p->a[sizeof p->a - 1] = 0; +} + + +void succ_2 (struct A *p, const struct A *q, int i) +{ + /* Same as above but with a conditional that cannot be eliminated. */ + if (i < 0) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + + p->a[sizeof p->a - 1] = 0; +} + + +/* Verify a strncpy call in a basic block with nul assignment in + the next successor block. */ +int next_succ (struct A *p, const struct A *q, int i, int j) +{ + /* Same as above but with a nested conditionals with else clauses. */ + if (i < 0) + { + if (j < 0) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + } + else + __builtin_strcpy (p->a, q->a); + + p->a[sizeof p->a - 1] = 0; + return 0; +} + + +int next_succ_1 (struct A *p, const struct A *q, int i, int j) +{ + /* Same as above but with a nested conditionals with else clauses. */ + if (i < 0) + { + if (j < 0) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + else + strncpy (p->a, q->a, sizeof p->a - 2); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + } + + p->a[sizeof p->a - 2] = 0; + return 1; +} + + +int next_succ_2 (struct A *p, const struct A *q, int i, int j) +{ + /* Same as above but with a nested conditionals with else clauses. */ + if (i < 0) + { + if (j < 0) + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + else + strncpy (p->a, q->a, sizeof p->a - 2); /* { dg-bogus "\\\[-Wstringop-truncation" } */ + } + else + __builtin_strcpy (p->a, q->a); + + p->a[sizeof p->a - 2] = 0; + return 2; +} + + +void cond_succ_warn (struct A *p, const struct A *q, int i) +{ + /* Verify that a conditional assignment doesn't suppress the warning. */ + strncpy (p->a, q->a, sizeof p->a - 1); /* { dg-warning "\\\[-Wstringop-truncation" } */ + + if (i < 0) + p->a[sizeof p->a - 1] = 0; +} + +void cond_succ_nowarn (struct A *p, const struct A *q) +{ + /* Verify that distinct but provably equivalent conditionals are + recognized and don't trigger the warning. */ + if (p != q) + strncpy (p->a, q->a, sizeof p->a - 1); + + if (p->a != q->a) + p->a[sizeof p->a - 1] = 0; +} Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 258088) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1856,8 +1856,33 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi avoid the truncation warning. */ gsi_next_nondebug (&gsi); gimple *next_stmt = gsi_stmt (gsi); + if (!next_stmt) + { + /* When there is no statement in the same basic block check + the immediate successor block. */ + if (basic_block bb = gimple_bb (stmt)) + { + if (single_succ_p (bb)) + { + /* For simplicity, ignore blocks with multiple outgoing + edges for now and only consider successor blocks along + normal edges. */ + edge e = EDGE_SUCC (bb, 0); + if (!(e->flags & EDGE_ABNORMAL)) + { + gsi = gsi_start_bb (e->dest); + next_stmt = gsi_stmt (gsi); + if (next_stmt && is_gimple_debug (next_stmt)) + { + gsi_next_nondebug (&gsi); + next_stmt = gsi_stmt (gsi); + } + } + } + } + } - if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt)) + if (next_stmt && is_gimple_assign (next_stmt)) { tree lhs = gimple_assign_lhs (next_stmt); tree_code code = TREE_CODE (lhs); --------------E8CA9F02FC586DDCAF58BA61--