On 2023/3/6 16:11, Richard Biener wrote:
On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefen...@gmail.com> wrote:
On 2023/3/2 18:45, Richard Biener wrote:
small.gcno: 648: block 2:`small.c':1, 3, 4, 6
small.gcno: 688: 01450000: 36:LINES
small.gcno: 700: block 3:`small.c':8, 9
small.gcno: 732: 01450000: 32:LINES
small.gcno: 744: block 5:`small.c':10
-small.gcno: 772: 01450000: 32:LINES
-small.gcno: 784: block 6:`small.c':12
-small.gcno: 812: 01450000: 36:LINES
-small.gcno: 824: block 7:`small.c':12, 13
+small.gcno: 772: 01450000: 36:LINES
+small.gcno: 784: block 6:`small.c':12, 13
+small.gcno: 816: 01450000: 32:LINES
+small.gcno: 828: block 8:`small.c':14
small.gcno: 856: 01450000: 32:LINES
-small.gcno: 868: block 8:`small.c':14
-small.gcno: 896: 01450000: 32:LINES
-small.gcno: 908: block 9:`small.c':17
+small.gcno: 868: block 9:`small.c':17
Looking at the CFG and the instrumentation shows
<bb 2> :
PROF_edge_counter_17 = __gcov0.f[0];
PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
__gcov0.f[0] = PROF_edge_counter_18;
[t.c:3:7] p_6 = 0;
[t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
<L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
<bb 3> :
# n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
# p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
[t.c:7:5] <L0>:
[t.c:8:15] p_12 = p_3 + 1;
[t.c:8:28] n_13 = n_1 + -1;
[t.c:8:28] if (n_13 != 0)
goto <bb 4>; [INV]
else
goto <bb 5>; [INV]
<bb 4> :
PROF_edge_counter_21 = __gcov0.f[2];
PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
__gcov0.f[2] = PROF_edge_counter_22;
[t.c:7:5] goto <bb 3>; [100.00%]
<bb 5> :
PROF_edge_counter_23 = __gcov0.f[3];
PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
__gcov0.f[3] = PROF_edge_counter_24;
[t.c:9:16] _14 = p_12;
[t.c:9:16] goto <bb 10>; [INV]
so the reason this goes wrong is that gcov associates the "wrong"
counter with the block containing
the 'case' label(s), for the case 0 it should have chosen the counter
from bb5 but it likely
computed the count of bb3?
It might be that ordering blocks differently puts the instrumentation
to different blocks or it
makes gcovs association chose different blocks but that means it's
just luck and not fixing
the actual issue?
To me it looks like the correct thing to investigate is switch
statement and/or case label
handling. One can also see that <L0> having line number 7 is wrong to
the extent that
the position of the label doesn't match the number of times it
executes in the source. So
placement of the label is wrong here, possibly caused by CFG cleanup
after CFG build
(but generally labels are not used for anything once the CFG is built
and coverage
instrumentation is late so it might fail due to us moving labels). It
might be OK to
avoid moving labels for --coverage but then coverage should possibly
look at edges
rather than labels?
Thanks, I investigated the Labels, it seems wrong at the beginning from
.gimple to .cfg very early quite like PR90574:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
.gimple:
int f (int s, int n)
[small.c:2:1] {
int D.2755;
int p;
[small.c:3:7] p = 0;
[small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0: <D.2743>,
[small.c:11:5] case 1: <D.2744>>
[small.c:7:5] <D.2743>: <= case label
<D.2748>: <= loop label
[small.c:8:13] p = p + 1;
[small.c:8:26] n = n + -1;
[small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
<D.2746>:
[small.c:9:14] D.2755 = p;
[small.c:9:14] return D.2755;
[small.c:11:5] <D.2744>:
<D.2751>:
[small.c:12:13] p = p + 1;
[small.c:12:26] n = n + -1;
[small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
<D.2749>:
[small.c:13:14] D.2755 = p;
[small.c:13:14] return D.2755;
<D.2756>:
[small.c:16:10] D.2755 = 0;
[small.c:16:10] return D.2755;
}
.cfg:
int f (int s, int n)
{
int p;
int D.2755;
<bb 2> :
[small.c:3:7] p = 0;
[small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0: <L0> [INV],
[small.c:11:5] case 1: <L3> [INV]>
<bb 3> :
[small.c:7:5] <L0>: <= case 0
[small.c:8:13 discrim 1] p = p + 1;
[small.c:8:26 discrim 1] n = n + -1;
[small.c:8:26 discrim 1] if (n != 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 4> :
[small.c:9:14] D.2755 = p;
[small.c:9:14] goto <bb 8>; [INV]
<bb 5> :
[small.c:11:5] <L3>: <= case 1
[small.c:12:13 discrim 1] p = p + 1;
[small.c:12:26 discrim 1] n = n + -1;
[small.c:12:26 discrim 1] if (n != 0)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
The labels are merged into the loop unexpected, so I tried below fix
for --coverage if two labels are not on same line to start new basic block:
index 10ca86714f4..b788198ac31 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t loc_prev = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (flag_test_coverage && !same_line_p (locus, &locus_e, loc_prev))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
Interesting. Note that in CFG cleanup we have the following condition
when deciding
whether to merge a forwarder block with the destination:
locus = single_succ_edge (bb)->goto_locus;
...
/* Now walk through the statements backward. We can ignore labels,
anything else means this is not a forwarder block. */
for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
{
gimple *stmt = gsi_stmt (gsi);
switch (gimple_code (stmt))
{
case GIMPLE_LABEL:
if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
return false;
if (!optimize
&& (gimple_has_location (stmt)
|| LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
&& gimple_location (stmt) != locus)
return false;
it would be nice to sync the behavior between CFG creation and this.
In particular
a missing piece of the puzzle is how CFG creation sets ->goto_locus of the edge
after your change and whether that goto_locus and the label locus
compare matches
your condition (the CFG cleanup one is even stricter but special cases
UNKNOWN_LOCATION).
I also notice the !optimize vs. flag_test_coverage condition mismatch.
That said - I think your change to stmt_starts_bb_p is definitely the
correct place to fix,
I'm wondering how to match up with CFG cleanup - possibly using
!optimize instead
of flag_test_coverage would even make sense for debugging as well - we should be
able to put a breakpoint on the label hitting once rather than once
each loop iteration.
Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
of gfortran cases execution failure with O0. Take gfortran.dg/index.f90 for
example:
.gimple:
__attribute__((fn spec (". ")))
void p ()
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
L.1:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
L.2:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
L.3:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
L.4:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
L.5:
[/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
L.6:
}
.cfg:
...
Removing basic block 7
;; basic block 7, loop depth 0
;; pred:
return;
;; succ: EXIT
;; 1 loops found
;;
;; Loop 0
;; header 0, latch 1
;; depth 0, outer -1
;; nodes: 0 1 2
;; 2 succs { }
__attribute__((fn spec (". ")))
void p ()
{
<bb 2> :
}
Due to the "return;" is removed in bb 7.
actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1 to
L.6
first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they
are all
EMPTY bb in make_edges_bb...
240│ /* To speed up statement iterator walks, we first purge dead labels.
*/
241│ cleanup_dead_labels ();
242│
243│ /* Group case nodes to reduce the number of edges.
244│ We do this after cleaning up dead labels because otherwise we miss
245│ a lot of obvious case merging opportunities. */
246│ group_case_labels ();
247│
248│ /* Create the edges of the flowgraph. */
249│ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
250├> make_edges ();
<bb 0> :
<bb 2> :
<bb 3> :
<bb 4> :
<bb 5> :
<bb 6> :
<bb 7> :
return;
<bb 1> :
Seems deadlock here as you said to set goto_locus as labels are removed before
edges are created, the case could pass if I comment out the function
cleanup_dead_labels(),
so also not call it when !optimize?
if (!!optimize)
cleanup_dead_labels ();
Attached v2 patch could pass regression test on
x86_64-linux-gnu/aarch64-linux-gnu.
From 575f845cbfc15b250f3debf2e2c99f95584e7afa Mon Sep 17 00:00:00 2001
From: Xionghu Luo <xionghu...@tencent.com>
Date: Tue, 28 Feb 2023 17:46:18 +0800
Subject: [PATCH v2] gcov: Fix "do-while" structure in case statement leads to
incorrect code coverage [PR93680]
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 (build_gimple_cfg): Don't delete labels if
!optimize.
(stmt_starts_bb_p): Check whether two labels are on same line.
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 <xionghu...@tencent.com>
---
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 +---
gcc/tree-cfg.cc | 10 ++++++++-
6 files changed, 37 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
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
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..41a269b5fe2 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -238,7 +238,8 @@ build_gimple_cfg (gimple_seq seq)
n_basic_blocks_for_fn (cfun));
/* To speed up statement iterator walks, we first purge dead labels. */
- cleanup_dead_labels ();
+ if (optimize)
+ cleanup_dead_labels ();
/* Group case nodes to reduce the number of edges.
We do this after cleaning up dead labels because otherwise we miss
@@ -2860,6 +2861,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
|| !DECL_ARTIFICIAL (gimple_label_label (plabel)))
return true;
+ location_t loc_prev = gimple_location (plabel);
+ location_t locus = gimple_location (label_stmt);
+ expanded_location locus_e = expand_location (locus);
+
+ if (!optimize && !same_line_p (locus, &locus_e, loc_prev))
+ return true;
+
cfg_stats.num_merged_labels++;
return false;
}
--
2.27.0
