Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680]

[Xionghu Luo <yinyuefengyi@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10338 bytes --]



On 2023/3/6 16:11, Richard Biener wrote:
> On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefengyi@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.

[-- Attachment #2: v2-0001-gcov-Fix-do-while-structure-in-case-statement-lea.patch --]
[-- Type: text/plain, Size: 4927 bytes --]

From 575f845cbfc15b250f3debf2e2c99f95584e7afa Mon Sep 17 00:00:00 2001
From: Xionghu Luo <xionghuluo@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 <xionghuluo@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