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

[Xionghu Luo <yinyuefengyi@gmail.com>]

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;
         }

.profile:

   <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;
   [small.c:3:7] p_6 = 0;
   [small.c:5:3] switch (s_7(D)) <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
   PROF_edge_counter_19 = __gcov0.f[1];
   PROF_edge_counter_20 = PROF_edge_counter_19 + 1;
   __gcov0.f[1] = PROF_edge_counter_20;

   <bb 4> :
   # n_1 = PHI <n_8(D)(3), [small.c:8:26 discrim 1] n_13(5)>
   # p_3 = PHI <[small.c:3:7] p_6(3), [small.c:8:13 discrim 1] p_12(5)>
   [small.c:8:13 discrim 1] p_12 = p_3 + 1;
   [small.c:8:26 discrim 1] n_13 = n_1 + -1;
   [small.c:8:26 discrim 1] if (n_13 != 0)
     goto <bb 5>; [INV]
   else
     goto <bb 6>; [INV]

   <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;
   goto <bb 4>; [100.00%]

   <bb 6> :
   [small.c:9:14] _14 = p_12;
   [small.c:9:14] goto <bb 12>; [INV]

   <bb 7> :
[small.c:11:5] <L3>:               <= case 1
   PROF_edge_counter_21 = __gcov0.f[2];
   PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
   __gcov0.f[2] = PROF_edge_counter_22;


   <bb 8> :
   # n_2 = PHI <n_8(D)(7), [small.c:12:26 discrim 1] n_10(9)>
   # p_4 = PHI <[small.c:3:7] p_6(7), [small.c:12:13 discrim 1] p_9(9)>
   [small.c:12:13 discrim 1] p_9 = p_4 + 1;
   [small.c:12:26 discrim 1] n_10 = n_2 + -1;
   [small.c:12:26 discrim 1] if (n_10 != 0)
     goto <bb 9>; [INV]
   else
     goto <bb 10>; [INV]

   <bb 9> :
   PROF_edge_counter_25 = __gcov0.f[4];
   PROF_edge_counter_26 = PROF_edge_counter_25 + 1;
   __gcov0.f[4] = PROF_edge_counter_26;
   goto <bb 8>; [100.00%]


then label lines are also correct now:

.c.gcov:

Lines executed:90.91% of 11
         -:    0:Source:small.c
         -:    0:Graph:small.gcno
         -:    0:Data:small.gcda
         -:    0:Runs:1
         2:    1:int f(int s, int n)
         -:    2:{
         2:    3:  int p = 0;
         -:    4:
         2:    5:  switch (s)
         -:    6:    {
         1:    7:    case 0:
         5:    8:      do { p++; } while (--n);
         1:    9:      return p;
         -:   10:
         1:   11:    case 1:
         5:   12:      do { p++; } while (--n);
         1:   13:      return p;
         -:   14:    }
         -:   15:
     #####:   16:  return 0;
         -:   17:}
         -:   18:
         1:   19:int main() { f(0, 5); f(1, 5);}