From patchwork Wed Mar 8 13:07:19 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xionghu Luo X-Patchwork-Id: 66128 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 968B03851ABD for ; Wed, 8 Mar 2023 13:07:58 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 968B03851ABD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1678280878; bh=1YFfRpkS6uvSB1udrhEM792I7nhPH9Hy/PvLJkLklfM=; h=Date:Subject:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=yI9Ew2dNcix0UHSTG8XAD3BU4ymuxTUVGbfZETw0SZqSJe1IPS6Qy7Md+OGmLGodI aE+PwCmE61vudsvM6pOwo9f/+QVnnQc2Zq7SeUsBIPQm04tvzRjPPz6aWI9SFznDig m7er8I/OkfJ7NAOMxNgHBpmOTgZCkGte9hX0HmwY= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pj1-x102c.google.com (mail-pj1-x102c.google.com [IPv6:2607:f8b0:4864:20::102c]) by sourceware.org (Postfix) with ESMTPS id C7E8C3850408; Wed, 8 Mar 2023 13:07:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C7E8C3850408 Received: by mail-pj1-x102c.google.com with SMTP id bo22so16433605pjb.4; Wed, 08 Mar 2023 05:07:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678280845; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=1YFfRpkS6uvSB1udrhEM792I7nhPH9Hy/PvLJkLklfM=; b=i7TrZ68SV9KkF4q21QL3YQnI6a/IPFIYONwdQmGZVdYWgPGH4rTs4rxox2OvodxGas YxzbPxZDhaqQstmUanzFll+FX760wumSwbNfd4LR7qCT+HMRyunvWOJ5vAXBIqVQGQeY Y5cQKMlJnjAoPk9FsGqjDCxN2TJq2c9ZMV77cORKctGZ9MHtKqhF5qpo1X0waRBYz1mS HLcQnwAf9CwWRBmHiqGIcaI1Y/RVymEGksO2VbKE+r4zogqBTpdIwcf0el67dVTiA/EL HL5MiUVpvJuS+ZQO3aUst9H7XbEJu1eL+TmTXVi9bXOm8EIrFR14gSTkxIoP/AuyP01v vibw== X-Gm-Message-State: AO0yUKXtELh5+kyqErkdq584bL3anDp41xmsv5gd66jIRmUrJGyhgpKi cSjjH2lHEGCeJE8lBb/ohZA= X-Google-Smtp-Source: AK7set+BC8GHkR3Qr6lu5FmstGAAZeLG5Un9dREkxbjrSjGMrwCgzIUPv/75dn+DUwSlWC1NHi8DXw== X-Received: by 2002:a17:902:eac2:b0:19c:fa22:e98a with SMTP id p2-20020a170902eac200b0019cfa22e98amr17306541pld.8.1678280844579; Wed, 08 Mar 2023 05:07:24 -0800 (PST) Received: from [192.168.255.10] ([117.143.173.67]) by smtp.gmail.com with ESMTPSA id x7-20020a1709029a4700b0019339f3368asm9923947plv.3.2023.03.08.05.07.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 Mar 2023 05:07:24 -0800 (PST) Message-ID: <79726845-749b-8e49-6c10-1f7930074ddf@gmail.com> Date: Wed, 8 Mar 2023 21:07:19 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: [PATCH v3] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] To: Richard Biener Cc: Richard Biener , gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, hubicka@ucw.cz References: <20230302022921.4055291-1-xionghuluo@tencent.com> <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> In-Reply-To: X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Xionghu Luo via Gcc-patches From: Xionghu Luo Reply-To: Xionghu Luo Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" On 2023/3/7 19:25, Richard Biener wrote: >>> It would be nice to avoid creating blocks / preserving labels we'll >>> immediately remove again. For that we do need some analysis >>> before creating basic-blocks that determines whether a label is >>> possibly reached by a non-falltru edge. >>> >> >> : >> p = 0; >> switch (s) , case 0: , case 1: > >> >> : >> : <= prev_stmt >> : <= stmt >> p = p + 1; >> n = n + -1; >> if (n != 0) goto ; else goto ; >> >> Check if is a case label and is a goto target then return true >> in stmt_starts_bb_p to start a new basic block? This would avoid creating and >> removing blocks, but cleanup_dead_labels has all bbs setup while >> stmt_starts_bb_p >> does't yet to iterate bbs/labels to establish label_for_bb[] map? > Yes. I think we'd need something more pragmatic before make_blocks (), > like re-computing TREE_USED of the label decls or computing a bitmap > of targeted labels (targeted by goto, switch or any other means). > > I'll note that doing a cleanup_dead_labels () like optimization before > we create blocks will help keeping LABEL_DECL_UID and thus > label_to_block_map dense. But it does look like a bit of > an chicken-and-egg problem and the question is how effective the > dead label removal is in practice. Tried to add function compute_target_labels(not sure whether the function name is suitable) in the front of make_blocks_1, now the fortran case doesn't create/removing blocks now, but I still have several questions: 1. I used hash_set to save the target labels instead of bitmap, as labels are tree type value instead of block index so bitmap is not good for it since we don't have LABEL_DECL_UID now? 2. Is the compute_target_labels still only for !optimize? And if we compute the target labels before create bbs, it is unnessary to guard the first cleanup_dead_labels under !optimize now, because the switch-case-do-while case already create new block for CASE_LABEL already. 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even labels_eh? PS1: The v3 patch will cause one test case fail: Number of regressions in total: 1 > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess errors) due to this exausting case has labels from L0 to L100001, they won't be optimized to a simple if-else expression like before... PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it so just skip these label... + case GIMPLE_GOTO: +#if 0 + if (!computed_goto_p (stmt)) + { + tree dest = gimple_goto_dest (stmt); + target_labels->add (dest); + } +#endif + break; Change the #if 0 to #if 1 result in: Number of regressions in total: 8 > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess errors) > FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors) > FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors) > FAIL: gfortran.dg/bound_2.f90 -O0 execution test > FAIL: gfortran.dg/bound_7.f90 -O0 execution test > FAIL: gfortran.dg/char_result_14.f90 -O0 execution test > FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test > FAIL: gfortran.dg/select_type_15.f03 -O0 execution test Paste the updated patch v3: v3: Add compute_target_labels and call it in the front of make_blocks_1. Start a new basic block if two labels have different location when test-coverage. Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for master? gcc/ChangeLog: PR gcov/93680 * tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in target_labels. (compute_target_labels): New function. (make_blocks_1): Call compute_target_labels. gcc/testsuite/ChangeLog: PR gcov/93680 * g++.dg/gcov/gcov-1.C: Correct counts. * gcc.misc-tests/gcov-4.c: Likewise. * gcc.misc-tests/gcov-pr85332.c: Likewise. * lib/gcov.exp: Also clean gcda if fail. * gcc.misc-tests/gcov-pr93680.c: New test. Signed-off-by: Xionghu Luo Signed-off-by: Xionghu Luo --- gcc/tree-cfg.cc | 68 ++++++++++++++++++++- gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +- gcc/testsuite/gcc.dg/analyzer/paths-4.c | 8 +-- gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +- gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++ gcc/testsuite/lib/gcov.exp | 4 +- 6 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index a9fcc7fd050..0f8efcf4aa3 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge, basic_block); static edge gimple_try_redirect_by_replacing_jump (edge, basic_block); /* Various helpers. */ -static inline bool stmt_starts_bb_p (gimple *, gimple *); +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set *); static int gimple_verify_flow_info (void); static void gimple_make_forwarder_block (edge); static gimple *first_non_label_stmt (basic_block); @@ -521,6 +521,59 @@ gimple_call_initialize_ctrl_altering (gimple *stmt) gimple_call_set_ctrl_altering (stmt, false); } +/* Compute target labels to save useful labels. */ +static void +compute_target_labels (gimple_seq seq, hash_set *target_labels) +{ + gimple *stmt = NULL; + gimple_stmt_iterator j = gsi_start (seq); + + while (!gsi_end_p (j)) + { + stmt = gsi_stmt (j); + + switch (gimple_code (stmt)) + { + case GIMPLE_COND: + { + gcond *cstmt = as_a (stmt); + tree true_label = gimple_cond_true_label (cstmt); + tree false_label = gimple_cond_false_label (cstmt); + target_labels->add (true_label); + target_labels->add (false_label); + } + break; + case GIMPLE_SWITCH: + { + gswitch *gstmt = as_a (stmt); + size_t i, n = gimple_switch_num_labels (gstmt); + tree elt, label; + for (i = 0; i < n; i++) + { + elt = gimple_switch_label (gstmt, i); + label = CASE_LABEL (elt); + target_labels->add (label); + } + } + break; + case GIMPLE_GOTO: +#if 0 + if (!computed_goto_p (stmt)) + { + tree dest = gimple_goto_dest (stmt); + target_labels->add (dest); + } +#endif + break; + + default: + break; + } + + gsi_next (&j); + } +} + /* Insert SEQ after BB and build a flowgraph. */ @@ -532,6 +585,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb) gimple *prev_stmt = NULL; bool start_new_block = true; bool first_stmt_of_seq = true; + hash_set target_labels; + + if (!optimize) + compute_target_labels (seq, &target_labels); while (!gsi_end_p (i)) { @@ -553,7 +610,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb) /* If the statement starts a new basic block or if we have determined in a previous pass that we need to create a new block for STMT, do so now. */ - if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt)) + if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels)) { if (!first_stmt_of_seq) gsi_split_seq_before (&i, &seq); @@ -2832,7 +2889,8 @@ simple_goto_p (gimple *t) label. */ static inline bool -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt) +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt, + hash_set *target_labels) { if (stmt == NULL) return false; @@ -2860,6 +2918,10 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt) || !DECL_ARTIFICIAL (gimple_label_label (plabel))) return true; + if (!optimize + && target_labels->contains (gimple_label_label (label_stmt))) + return true; + cfg_stats.num_merged_labels++; return false; } diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C index ee383b480a8..01e7084fb03 100644 --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C @@ -263,7 +263,7 @@ test_switch (int i, int j) case 2: result = do_something (1024); break; - case 3: /* count(3) */ + case 3: /* count(2) */ case 4: /* branch(67) */ if (j == 2) /* count(3) */ diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-4.c b/gcc/testsuite/gcc.dg/analyzer/paths-4.c index b72e658739e..fdf33e68d0c 100644 --- a/gcc/testsuite/gcc.dg/analyzer/paths-4.c +++ b/gcc/testsuite/gcc.dg/analyzer/paths-4.c @@ -35,18 +35,18 @@ int test_2 (struct state *s) do_stuff (s, 0); break; case 1: - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */ do_stuff (s, 17); break; case 2: - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */ do_stuff (s, 5); break; case 3: - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */ return 42; case 4: - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed enode" } */ + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed enode" } */ return -3; } } diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c index 73e50b19fc7..b37e760910c 100644 --- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c @@ -7,7 +7,7 @@ int doit(int sel, int n, void *p) switch (sel) { - case 0: /* count(3) */ + case 0: /* count(1) */ do {*p0 += *p0;} while (--n); /* count(3) */ return *p0 == 0; /* count(1) */ diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c new file mode 100644 index 00000000000..2fe340c4011 --- /dev/null +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c @@ -0,0 +1,24 @@ +/* { dg-options "-fprofile-arcs -ftest-coverage" } */ +/* { dg-do run { target native } } */ + +int f(int s, int n) +{ + int p = 0; + + switch (s) + { + case 0: /* count(1) */ + do { p++; } while (--n); /* count(5) */ + return p; /* count(1) */ + + case 1: /* count(1) */ + do { p++; } while (--n); /* count(5) */ + return p; /* count(1) */ + } + + return 0; +} + +int main() { f(0, 5); f(1, 5); return 0; } + +/* { dg-final { run-gcov gcov-pr93680.c } } */ diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp index 80e74aeb220..07e1978d25d 100644 --- a/gcc/testsuite/lib/gcov.exp +++ b/gcc/testsuite/lib/gcov.exp @@ -424,9 +424,7 @@ proc run-gcov { args } { } if { $tfailed > 0 } { fail "$testname gcov: $lfailed failures in line counts, $bfailed in branch percentages, $cfailed in return percentages, $ifailed in intermediate format" - if { $xfailed } { - clean-gcov $testcase - } + clean-gcov $testcase } else { pass "$testname gcov" clean-gcov $testcase