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

  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
  

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

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

On Thu, Mar 2, 2023 at 11:22 AM Xionghu Luo <yinyuefengyi@gmail.com> wrote:
>
>
>
> 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

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?

Richard.

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

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

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.

Thanks,
Richard.

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

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

On Tue, 7 Mar 2023, Xionghu Luo wrote:

> 
> 
> 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.

OK, the issue is that make_edges_bb does nothing for an empty block
but it should at least create a fallthru edge here.  Thus,

  if (!last)
    fallthru = true;

  else
    switch (gimple_code (last))
      {

instead of simply returning if (!last).  The alternative would be
to make sure that cleanup_dead_labels preserves at least one
statement in a block.

Looking at the testcases I wonder if preserving all the fallthru labels
is really necessary - for coverage we should have a counter ready.  For
the testcase we arrive with

L.1:
L.2:
L.3:
L.4:
i = 1;

where the frontend simplified things but put labels at each line.
I suppose we could optimize this by re-computing TREE_USED and only
splitting before labels reached by a control statement?  That would
cover the backedge case in the original testcase.  cleanup_dead_labels
does something like that already.

> 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 ();

That probably makes sense.  Looking at group_case_labels () that also
seems to do unwanted things (to debugging and coverage), its comment
says that for

 switch (i)
 {
 case 1:
   /* fallthru */
 case 2:
   /* fallthru */
 case 3:
   k = 0;

it would replace that with

 case 1..3:
   k = 0;

but that also fails to produce correct coverage, right?  Likewise
setting breakpoints.

Does preserving the labels help setting a goto_locus for the
fallthru edges?  I don't see any code doing that, so
CFG cleanup will remove the forwarders we created again.

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.

Richard.

> 
> Attached v2 patch could pass regression test on
> x86_64-linux-gnu/aarch64-linux-gnu.
>
  

On 2023/3/7 16:53, Richard Biener wrote:
> On Tue, 7 Mar 2023, Xionghu Luo wrote:

>> 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.
> 
> OK, the issue is that make_edges_bb does nothing for an empty block
> but it should at least create a fallthru edge here.  Thus,
> 
>    if (!last)
>      fallthru = true;
> 
>    else
>      switch (gimple_code (last))
>        {
> 
> instead of simply returning if (!last).  The alternative would be
> to make sure that cleanup_dead_labels preserves at least one
> statement in a block.
> 
> Looking at the testcases I wonder if preserving all the fallthru labels
> is really necessary - for coverage we should have a counter ready.  For
> the testcase we arrive with
> 
> L.1:
> L.2:
> L.3:
> L.4:
> i = 1;

It was:

<bb 0> :

<bb 2> :
L.1:

<bb 3> :
L.2:

<bb 4> :
L.3:

<bb 5> :
L.4:

<bb 6> :
L.5:

<bb 7> :
L.6:
return;

<bb 1> :

before the second call of cleanup_dead_labels, after it, all labels are
removed, then tree_forwarder_block_p remove all forworders.  Yes, it
creates blocks and remove blocks immediately...

> 
> where the frontend simplified things but put labels at each line.
> I suppose we could optimize this by re-computing TREE_USED and only
> splitting before labels reached by a control statement?  That would
> cover the backedge case in the original testcase.  cleanup_dead_labels
> does something like that already.
> 
>> 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 ();
> 
> That probably makes sense.  Looking at group_case_labels () that also
> seems to do unwanted things (to debugging and coverage), its comment
> says that for
> 
>   switch (i)
>   {
>   case 1:
>     /* fallthru */
>   case 2:
>     /* fallthru */
>   case 3:
>     k = 0;
> 
> it would replace that with
> 
>   case 1..3:
>     k = 0;
> 
> but that also fails to produce correct coverage, right?  Likewise
> setting breakpoints.

Yes.  Should also exclude this.

> 
> Does preserving the labels help setting a goto_locus for the
> fallthru edges?  I don't see any code doing that, so
> CFG cleanup will remove the forwarders we created again.

For the backedge case with switch-case-do-while, tree_forwarder_block_p
returns false when iterating the statement check.
The new created <bb 3> with only one case label instruction still owns
location information in it, so CFG cleanup won't remove the forwarders.

  390│   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
  391│     {
  392│       gimple *stmt = gsi_stmt (gsi);
  393│
  394│       switch (gimple_code (stmt))
  395│     {
  396│     case GIMPLE_LABEL:
  397│       if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
  398│         return false;
  399│       if (!optimize
  400│           && (gimple_has_location (stmt)
  401│           || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
  402│           && gimple_location (stmt) != locus)
  403├>        return false;
  404│       break;


(gdb) ps stmt
<L0>:
(gdb) p gimple_location (stmt)
$154 = 2147483656
(gdb) pel $154
{file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp = false}
(gdb)
(gdb) pbb bb
;; basic block 3, loop depth 0
;;  pred:       2
<L0>:
;;  succ:       4

> 
> 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?


Thanks,
Xionghu
  

On Tue, 7 Mar 2023, Xionghu Luo wrote:

> 
> 
> On 2023/3/7 16:53, Richard Biener wrote:
> > On Tue, 7 Mar 2023, Xionghu Luo wrote:
> 
> >> 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.
> > 
> > OK, the issue is that make_edges_bb does nothing for an empty block
> > but it should at least create a fallthru edge here.  Thus,
> > 
> >    if (!last)
> >      fallthru = true;
> > 
> >    else
> >      switch (gimple_code (last))
> >        {
> > 
> > instead of simply returning if (!last).  The alternative would be
> > to make sure that cleanup_dead_labels preserves at least one
> > statement in a block.
> > 
> > Looking at the testcases I wonder if preserving all the fallthru labels
> > is really necessary - for coverage we should have a counter ready.  For
> > the testcase we arrive with
> > 
> > L.1:
> > L.2:
> > L.3:
> > L.4:
> > i = 1;
> 
> It was:
> 
> <bb 0> :
> 
> <bb 2> :
> L.1:
> 
> <bb 3> :
> L.2:
> 
> <bb 4> :
> L.3:
> 
> <bb 5> :
> L.4:
> 
> <bb 6> :
> L.5:
> 
> <bb 7> :
> L.6:
> return;
> 
> <bb 1> :
> 
> before the second call of cleanup_dead_labels, after it, all labels are
> removed, then tree_forwarder_block_p remove all forworders.  Yes, it
> creates blocks and remove blocks immediately...
> 
> > 
> > where the frontend simplified things but put labels at each line.
> > I suppose we could optimize this by re-computing TREE_USED and only
> > splitting before labels reached by a control statement?  That would
> > cover the backedge case in the original testcase.  cleanup_dead_labels
> > does something like that already.
> > 
> >> 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 ();
> > 
> > That probably makes sense.  Looking at group_case_labels () that also
> > seems to do unwanted things (to debugging and coverage), its comment
> > says that for
> > 
> >   switch (i)
> >   {
> >   case 1:
> >     /* fallthru */
> >   case 2:
> >     /* fallthru */
> >   case 3:
> >     k = 0;
> > 
> > it would replace that with
> > 
> >   case 1..3:
> >     k = 0;
> > 
> > but that also fails to produce correct coverage, right?  Likewise
> > setting breakpoints.
> 
> Yes.  Should also exclude this.
> 
> > 
> > Does preserving the labels help setting a goto_locus for the
> > fallthru edges?  I don't see any code doing that, so
> > CFG cleanup will remove the forwarders we created again.
> 
> For the backedge case with switch-case-do-while, tree_forwarder_block_p
> returns false when iterating the statement check.
> The new created <bb 3> with only one case label instruction still owns
> location information in it, so CFG cleanup won't remove the forwarders.
> 
>  390│   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
>  391│     {
>  392│       gimple *stmt = gsi_stmt (gsi);
>  393│
>  394│       switch (gimple_code (stmt))
>  395│     {
>  396│     case GIMPLE_LABEL:
>  397│       if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
>  398│         return false;
>  399│       if (!optimize
>  400│           && (gimple_has_location (stmt)
>  401│           || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
>  402│           && gimple_location (stmt) != locus)
>  403├>        return false;
>  404│       break;
> 
> 
> (gdb) ps stmt
> <L0>:
> (gdb) p gimple_location (stmt)
> $154 = 2147483656
> (gdb) pel $154
> {file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp
> = false}
> (gdb)
> (gdb) pbb bb
> ;; basic block 3, loop depth 0
> ;;  pred:       2
> <L0>:
> ;;  succ:       4
> 
> > 
> > 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.

Richard.