From: Xionghu Luo <yinyuefengyi@gmail.com>
To: Richard Biener <rguenther@suse.de>
Cc: Richard Biener <richard.guenther@gmail.com>,
gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, hubicka@ucw.cz
Subject: [PATCH v3] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]
Date: Wed, 8 Mar 2023 21:07:19 +0800 [thread overview]
Message-ID: <79726845-749b-8e49-6c10-1f7930074ddf@gmail.com> (raw)
In-Reply-To: <nycvar.YFH.7.77.849.2303071109550.18795@jbgna.fhfr.qr>
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.
>>>
>>
>> <bb 2> :
>> p = 0;
>> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>>
>> <bb 3> :
>> <L0>: <= prev_stmt
>> <D.2748>: <= stmt
>> p = p + 1;
>> n = n + -1;
>> if (n != 0) goto <D.2748>; else goto <D.2746>;
>>
>> Check if <L0> is a case label and <D.2748> 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<tree> 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 <xionghuluo@tencent.com>
---
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<tree> *);
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<tree> *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 <gcond *> (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 <gswitch *> (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<tree> 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<tree> *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
--
2.27.0
next prev parent reply other threads:[~2023-03-08 13:07 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-02 2:29 [PATCH 1/2] " Xionghu Luo
2023-03-02 2:29 ` [PATCH 2/2] gcov: Fix incorrect gimple line LOCATION [PR97923] Xionghu Luo
2023-03-02 8:16 ` Richard Biener
2023-03-02 9:43 ` Xionghu Luo
2023-03-02 10:02 ` Richard Biener
2023-03-02 8:41 ` [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] Richard Biener
2023-03-02 10:22 ` Xionghu Luo
2023-03-02 10:45 ` Richard Biener
2023-03-06 7:22 ` Xionghu Luo
2023-03-06 8:11 ` Richard Biener
2023-03-07 7:41 ` Xionghu Luo
2023-03-07 8:53 ` Richard Biener
2023-03-07 10:26 ` Xionghu Luo
2023-03-07 11:25 ` Richard Biener
2023-03-08 13:07 ` Xionghu Luo [this message]
2023-03-09 12:02 ` [PATCH v3] " Richard Biener
2023-03-14 2:06 ` [PATCH v4] " Xionghu Luo
2023-03-21 11:18 ` Richard Biener
2023-03-15 10:07 ` Xionghu Luo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=79726845-749b-8e49-6c10-1f7930074ddf@gmail.com \
--to=yinyuefengyi@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=luoxhu@gcc.gnu.org \
--cc=rguenther@suse.de \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).