From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 498 invoked by alias); 27 Feb 2018 00:47:07 -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 482 invoked by uid 89); 27 Feb 2018 00:47:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.7 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; Tue, 27 Feb 2018 00:47:04 +0000 Received: by mail-ot0-f196.google.com with SMTP id g97so2238661otg.13 for ; Mon, 26 Feb 2018 16:47:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=WipPp6Guz3xHk6uHjEiGZWT/ngmSTcTusU3Uytu8b3w=; b=Rl/yL609vqNrV19i4ez5PKRPlLmHfB+/nKkiGLRA6+Gdl6XJISognkOdjFJS+WJ9kH 6m8Yz6pbjUiQBxIl2C4F1NgMaiAbzbvetgyWqZ+kXEJ5cWLMQ4D2hL5ZlP79t3tNfQWB 2XRkkoPF8zvN+Et8S173ClO3iz7DiA12SJiTxyh5MP6td9nCNgIsFNvhZpigcOBHuGNn jW3NR4k+kbOFwh4FreNKX5+HcPIS9qT++00wJacVlRFafa3pZktoUe2o5/1/ZPnVW6hL lPA16gyvJ/MeJV33aKkp+byF/CDMY8Kfeu37bTisPsIdcu0WSIKRkCR/UkZdB3yw2HMd v5wA== X-Gm-Message-State: APf1xPDnXL1KPLkWsNVg+BdVRiU3QAX81H/qii4U5Xizmdc7m0gYI2qZ vkEcIvC/+/j5vXupba5Q5iZXug== X-Google-Smtp-Source: AH8x224oPjj7fIXOAqNI2rtCs4/C99QgJybf9rjEhpgQ71k/UZSJW31CQ8Q3Ut0aXEG0w3jpDxEjvg== X-Received: by 10.157.64.225 with SMTP id t30mr8832394oti.293.1519692422667; Mon, 26 Feb 2018 16:47:02 -0800 (PST) Received: from localhost.localdomain (75-171-228-29.hlrn.qwest.net. [75.171.228.29]) by smtp.gmail.com with ESMTPSA id r58sm5398063otb.58.2018.02.26.16.47.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 26 Feb 2018 16:47:01 -0800 (PST) Subject: Re: [PING] [PATCH] consider successor blocks when avoiding -Wstringop-truncation (PR 84468) To: Jeff Law , Gcc Patch List References: From: Martin Sebor Message-ID: <7116c7c3-43f3-0784-a05b-f68253cd7e5f@gmail.com> Date: Tue, 27 Feb 2018 00:47: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="------------8E346803E98BB062A1225D12" X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg01473.txt.bz2 This is a multi-part message in MIME format. --------------8E346803E98BB062A1225D12 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2027 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). Martin --------------8E346803E98BB062A1225D12 Content-Type: text/x-patch; name="gcc-84468.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-84468.diff" Content-length: 11440 PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy gcc/ChangeLog: PR tree-optimization/84468 * tree-ssa-strlen.c (suppress_stringop_trunc): New function to also consider successor basic blocks. (maybe_diag_stxncpy_trunc): Call it. gcc/testsuite/ChangeLog: PR tree-optimization/84468 * gcc.dg/Wstringop-truncation.c: New test. * gcc.dg/Wstringop-truncation-2.c: New test. Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 258016) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1781,6 +1781,83 @@ is_strlen_related_p (tree src, tree len) return false; } +/* Return true if -Wstringop-truncation for statement STMT should be + suppressed. GSI is an interator initially pointing at STMT and + set to subsequent statements on recursive calls by the function. + REF refers to the object being referred to. VISITED is a bitmap + of visited basic blocks to prevent infinite recursion. */ + +static bool +suppress_stringop_trunc (gimple *stmt, gimple_stmt_iterator gsi, tree ref, + bitmap visited) +{ + if (gsi_end_p (gsi)) + return false; + + gimple *cur_stmt = gsi_stmt (gsi); + if (is_gimple_debug (cur_stmt) || stmt == cur_stmt) + 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 (cur_stmt)) + { + /* Return if the basic block has already been visited. */ + if (!bitmap_set_bit (visited, bb->index)) + return false; + + edge e; + edge_iterator ei; + FOR_EACH_EDGE (e, ei, bb->succs) + { + if (e->flags & EDGE_ABNORMAL) + continue; + + basic_block nextbb = e->dest; + gimple_stmt_iterator it = gsi_start_bb (nextbb); + if (suppress_stringop_trunc (stmt, it, ref, visited)) + return true; + } + } + + return false; + } + + if (!is_gimple_assign (next_stmt)) + return false; + + tree lhs = gimple_assign_lhs (next_stmt); + tree_code code = TREE_CODE (lhs); + if (code == ARRAY_REF || code == MEM_REF) + lhs = TREE_OPERAND (lhs, 0); + + tree func = gimple_call_fndecl (stmt); + if (DECL_FUNCTION_CODE (func) == BUILT_IN_STPNCPY) + { + tree ret = gimple_call_lhs (stmt); + if (ret && operand_equal_p (ret, lhs, 0)) + return true; + } + + /* Determine the base address and offset of the reference, + ignoring the innermost array index. */ + if (TREE_CODE (ref) == ARRAY_REF) + ref = TREE_OPERAND (ref, 0); + + poly_int64 dstoff; + tree dstbase = get_addr_base_and_unit_offset (ref, &dstoff); + + poly_int64 lhsoff; + tree lhsbase = get_addr_base_and_unit_offset (lhs, &lhsoff); + return (lhsbase + && dstbase + && known_eq (dstoff, lhsoff) + && operand_equal_p (dstbase, lhsbase, 0)); +} + /* Called by handle_builtin_stxncpy and by gimple_fold_builtin_strncpy in gimple-fold.c. Check to see if the specified bound is a) equal to the size of @@ -1846,49 +1923,22 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi if (TREE_CODE (dstdecl) == ADDR_EXPR) dstdecl = TREE_OPERAND (dstdecl, 0); - /* If the destination refers to a an array/pointer declared nonstring + /* If the destination refers to an array/pointer declared nonstring return early. */ tree ref = NULL_TREE; if (get_attr_nonstring_decl (dstdecl, &ref)) return false; - /* Look for dst[i] = '\0'; after the stxncpy() call and if found - avoid the truncation warning. */ - gsi_next_nondebug (&gsi); - gimple *next_stmt = gsi_stmt (gsi); + { + /* Look for dst[i] = '\0'; after the stxncpy() call and if found + avoid the truncation warning. */ + bitmap visited = BITMAP_ALLOC (NULL); + bool suppress = suppress_stringop_trunc (stmt, gsi, ref, visited); + BITMAP_FREE (visited); + if (suppress) + return false; + } - if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt)) - { - tree lhs = gimple_assign_lhs (next_stmt); - tree_code code = TREE_CODE (lhs); - if (code == ARRAY_REF || code == MEM_REF) - lhs = TREE_OPERAND (lhs, 0); - - tree func = gimple_call_fndecl (stmt); - if (DECL_FUNCTION_CODE (func) == BUILT_IN_STPNCPY) - { - tree ret = gimple_call_lhs (stmt); - if (ret && operand_equal_p (ret, lhs, 0)) - return false; - } - - /* Determine the base address and offset of the reference, - ignoring the innermost array index. */ - if (TREE_CODE (ref) == ARRAY_REF) - ref = TREE_OPERAND (ref, 0); - - poly_int64 dstoff; - tree dstbase = get_addr_base_and_unit_offset (ref, &dstoff); - - poly_int64 lhsoff; - tree lhsbase = get_addr_base_and_unit_offset (lhs, &lhsoff); - if (lhsbase - && dstbase - && known_eq (dstoff, lhsoff) - && operand_equal_p (dstbase, lhsbase, 0)) - return false; - } - int prec = TYPE_PRECISION (TREE_TYPE (cnt)); wide_int lenrange[2]; if (strinfo *sisrc = sidx > 0 ? get_strinfo (sidx) : NULL) 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/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; +} --------------8E346803E98BB062A1225D12--