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 16:41, Richard Biener wrote:
> On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> When spliting edge with self loop, the split edge should be placed just next to
>> the edge_in->src, otherwise it may generate different position latch bbs for
>> two consecutive self loops.  For details, please refer to:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4
>>
>> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
>> master?
>>
>> gcc/ChangeLog:
>>
>>          PR gcov/93680
>>          * tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop.
>>
>> gcc/testsuite/ChangeLog:
>>
>>          PR gcov/93680
>>          * gcc.misc-tests/gcov-pr93680.c: New test.
>>
>> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
>> ---
>>   gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++
>>   gcc/tree-cfg.cc                             |  2 +-
>>   2 files changed, 25 insertions(+), 1 deletion(-)
>>   create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>>
>> 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..b2bf9e626fc
>> --- /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(5) */
>> +      do { p++; } while (--n); /* count(5) */
>> +      return p; /* count(1) */
>> +
>> +    case 1: /* count(5) */
>> +      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/tree-cfg.cc b/gcc/tree-cfg.cc
>> index a9fcc7fd050..6fa1d83d366 100644
>> --- a/gcc/tree-cfg.cc
>> +++ b/gcc/tree-cfg.cc
>> @@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in)
>>     if (dest_prev)
>>       {
>>         edge e = find_edge (dest_prev, dest);
>> -      if (e && !(e->flags & EDGE_COMPLEX))
>> +      if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest)
> 
> I think this should eventually apply to all backedge edge_in, correct?
>   But of course
> we cannot easily test for this here.
> 
> Still since this affects ordering in the {next,prev}_bb chain only but not CFG
> semantics I wonder how it can affect coverage?  Isn't it only by chance that
> this block order survives?

For case:

1 int f(int s, int n)
2 {
3  int p = 0;
4  int q = 0;
5
6  switch (s)
7    {
8    case 0:
9      do { p++; } while (--n);
10      return p;
11
12    case 1:
13      do { p++; } while (--n);
14      return p;
15    }
16
17  return 0;
18 }
19
20 int main() { f(0, 5); f(1, 5);}


current GCC generates:

<bb 2> :
...

  <bb 3> :                <= first loop
...
     goto <bb 4>; [INV]
   else
     goto <bb 5>; [INV]

   <bb 4> :               <= first latch bb
   goto <bb 3>; [100.00%]

   <bb 5> :
...
   goto <bb 10>; [INV]

   <bb 6> :               <= second latch bb

   <bb 7> :                <= second loop
...
     goto <bb 6>; [INV]
   else
     goto <bb 8>; [INV]


<bb 4> and <bb 6> are created by split_edge->split_edge_bb_loc, <bb 4>
is located after the loop, but <bb 6> is located before the loop.

First call of split_edge_bb_loc, the dest_prev is <bb 2>, and find_edge
did find a edge from <bb 2> to <bb 3>, the returned afte_bb is <bb 3>, so
latch <bb 4> is put after the loop

but second call of split_edge_bb_loc, the dest_prev is <bb 5>, so find_edge
return 0, and the returned after_bb is <bb 5>, then the created latch <bb 6>
is put before the loop...

Different latch bb position caused different gcno, while gcov has poor
information and not that smart to recognize it:(, is it reasonable to keep
this kind of loops same order?


  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



> 
> For the case when both edge_in->src has more than one successor and
> edge_in->dest has more than one predecessor there isn't any good heuristic
> to make printing the blocks in chain order "nice" (well, the backedge
> one maybe).
> 
> But as said - this order shouldn't have any effect on semantics ...
> 
>>          return edge_in->src;
>>       }
>>     return dest_prev;
>> --
>> 2.27.0
>>