From patchwork Wed Mar 15 10:07:24 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Xionghu Luo X-Patchwork-Id: 66403 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 D78DE3858296 for ; Wed, 15 Mar 2023 10:08:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D78DE3858296 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1678874890; bh=YzwvChKajnr3+16IUFeYA8xJNul6uYhYcnfXzteNlhc=; 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=RWlXvimrdUwNO51og0Q7BYaNXvgo2ash9TmwuG5A1X/lLmbMxYCmdHUQXyBALbHU5 HYVBVLgj1FF2aQb5JegWNTBIcTP3Giyfz9E/a7C8yh9mH4yi/wQLvpQMaokpp7o7hP kyIKXw4fGC6an9/8hTTgz5c+WN0vn6JUxDdvpSiA= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pj1-x102b.google.com (mail-pj1-x102b.google.com [IPv6:2607:f8b0:4864:20::102b]) by sourceware.org (Postfix) with ESMTPS id CB3413858D39; Wed, 15 Mar 2023 10:07:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CB3413858D39 Received: by mail-pj1-x102b.google.com with SMTP id om3-20020a17090b3a8300b0023efab0e3bfso1153826pjb.3; Wed, 15 Mar 2023 03:07:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678874860; h=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=YzwvChKajnr3+16IUFeYA8xJNul6uYhYcnfXzteNlhc=; b=xzakTHPATGvPIbuVZ83Ich+3H5aoxiWDd7mlR/ZMBuzrgmD6YMVTRKkPrH1XJu02go CedDUR2I7vFuQSMHzy+LRV3CXDSnF7U2RH/QXtSGpJBlcWC4yo9FZ0NRYTAuzSs7iJ6R /VpLZsVuBRUI/QjoPX9TXUR6ITUxmxB1aPz9gJ+AvrTczJ/J0u2uoRYLXW4fcwQ50zCd JcY8om5yay9Jj6XKsYUXYRU4cjphUNZy7ER2ivY/UwXNGhJRpEviTHgr64bSafpKDtn2 zL0oz/8PsMT7yTQe4bv+90zBEonNhq2UDKf6W64ydg5JCAfpUkQXiQrMPZ7jZFEz5MUu MXhA== X-Gm-Message-State: AO0yUKWtOHDM6JA3ZvXfv7JF6OWZGx7VtMbnWPFfB7tgFYlAZLcdm/ZZ UpivfF//jbeba7Zgecxlvzw= X-Google-Smtp-Source: AK7set/IbMvyyjT1qbsnVHJaGnXN0HwGnwQ3UFw6Dpa44bn/NwgwvpX5wDimoqxNDu2BBUHZqi5EZg== X-Received: by 2002:a17:90b:4acb:b0:237:44c3:df0b with SMTP id mh11-20020a17090b4acb00b0023744c3df0bmr43161386pjb.32.1678874859560; Wed, 15 Mar 2023 03:07:39 -0700 (PDT) Received: from [192.168.255.10] ([103.7.29.32]) by smtp.gmail.com with ESMTPSA id u13-20020a17090a890d00b00233df90227fsm1005138pjn.18.2023.03.15.03.07.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Mar 2023 03:07:39 -0700 (PDT) Message-ID: <977b3846-f1fc-77a5-c1ee-367dd947ed44@gmail.com> Date: Wed, 15 Mar 2023 18:07:24 +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 v4] 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> <79726845-749b-8e49-6c10-1f7930074ddf@gmail.com> In-Reply-To: X-Spam-Status: No, score=-10.1 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/9 20:02, Richard Biener wrote: > On Wed, 8 Mar 2023, Xionghu Luo wrote: > >> >> >> 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? > > We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of > hash_set vs. bitmap is somewhat arbitrary here. The real cost is > the extra walk over all stmts. > >> 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. > > OK. > >> 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? > > I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and > coverage already? > >> 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... > > Hmm, that's somewhat unexpected. > >> >> 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... > > Please investigate, we might be missing a corner case here. > I think the *previous fix* for labels “in the middle of block” is *incorrect*, it should be handled in make_edges_bb when a basic block only has Label in it, just create a fallthrough edge for it to avoid wrong cfg and unreachable trap generated? @@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index) bool fallthru = false; int ret = 0; + if (!optimize && !last) + { + make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + return 0; + } + if (!last) return ret; With the fix, the attached version could pass bootstrap and regression test on x86_64-linux-gnu. From ec505cc7952707db805802af83dd82776a1d949f Mon Sep 17 00:00:00 2001 From: Xionghu Luo Date: Tue, 28 Feb 2023 17:46:18 +0800 Subject: [PATCH v4] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] v4: Address comments. 4.1. Handle GIMPLE_GOTO and GIMPLE_ASM. 4.2. Fix failure of limit-caselabels.c (labels on same line), pointer_array_1.f90 (unused labels) etc. v3: Add compute_target_labels and call it in the front of make_blocks_1. v2: Check whether two locus are on same line. 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. (same_line_p): Return false if two locus are both UNKOWN_LOCATION. 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 --- gcc/tree-cfg.cc | 97 ++++++++++++++++++++- gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +- gcc/testsuite/gcc.misc-tests/gcov-4.c | 2 +- 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, 122 insertions(+), 9 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..7b185200aaf 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,68 @@ 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 (!computed_goto_p (stmt)) + { + tree dest = gimple_goto_dest (stmt); + target_labels->add (dest); + } + break; + case GIMPLE_ASM: + { + gasm *asm_stmt = as_a (stmt); + int i, n = gimple_asm_nlabels (asm_stmt); + for (i = 0; i < n; ++i) + { + tree cons = gimple_asm_label_op (asm_stmt, i); + target_labels->add (cons); + } + } + break; + + default: + break; + } + + gsi_next (&j); + } +} + /* Insert SEQ after BB and build a flowgraph. */ @@ -532,6 +594,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 +619,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); @@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb) codes. */ gimple_set_bb (stmt, bb); + if (!optimize && gimple_code (stmt) == GIMPLE_LABEL) + target_labels.add (gimple_label_label (as_a (stmt))); + /* If STMT is a basic block terminator, set START_NEW_BLOCK for the next iteration. */ if (stmt_ends_bb_p (stmt)) @@ -853,6 +922,12 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index) bool fallthru = false; int ret = 0; + if (!optimize && !last) + { + make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + return 0; + } + if (!last) return ret; @@ -1152,6 +1227,10 @@ same_line_p (location_t locus1, expanded_location *from, location_t locus2) { expanded_location to; + if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION + && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION) + return false; + if (locus1 == locus2) return true; @@ -2832,7 +2911,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 +2940,17 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt) || !DECL_ARTIFICIAL (gimple_label_label (plabel))) return true; + location_t prev_locus = gimple_location (plabel); + location_t locus = gimple_location (label_stmt); + expanded_location locus_e = expand_location (locus); + + if (!optimize + && target_labels->contains (gimple_label_label (label_stmt)) + && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION + || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION) + && !same_line_p (locus, &locus_e, prev_locus)) + 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.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c index da7929ef7fc..792cda8cfce 100644 --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c @@ -122,7 +122,7 @@ top: } else { -else_: /* count(1) */ +else_: /* count(2) */ j = do_something (j); /* count(2) */ if (j) /* count(2) */ { 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