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

  On 2023/3/7 19:25, Richard Biener wrote:
>>> 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.

Tried to add function compute_target_labels(not sure whether the function
name is suitable) in the front of make_blocks_1, now the fortran case doesn't
create/removing blocks now, but I still have several questions:

  1. I used hash_set<tree> to save the target labels instead of bitmap, as labels
are tree type value instead of block index so bitmap is not good for it since
we don't have LABEL_DECL_UID now?
  2. Is the compute_target_labels still only for !optimize?  And if we compute
the target labels before create bbs, it is unnessary to guard the first
cleanup_dead_labels under !optimize now, because the switch-case-do-while
case already create new block for CASE_LABEL already.
  3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
labels_eh?


PS1: The v3 patch will cause one test case fail:

Number of regressions in total: 1
> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess errors)

due to this exausting case has labels from L0 to L100001, they won't be optimized
to a simple if-else expression like before...


PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it so
just skip these label...


+       case GIMPLE_GOTO:
+#if 0
+         if (!computed_goto_p (stmt))
+           {
+             tree dest = gimple_goto_dest (stmt);
+             target_labels->add (dest);
+           }
+#endif
+         break;

Change the #if 0 to #if 1 result in:

Number of regressions in total: 8
> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess errors)
> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> FAIL: gfortran.dg/bound_2.f90   -O0  execution test
> FAIL: gfortran.dg/bound_7.f90   -O0  execution test
> FAIL: gfortran.dg/char_result_14.f90   -O0  execution test
> FAIL: gfortran.dg/pointer_array_1.f90   -O0  execution test
> FAIL: gfortran.dg/select_type_15.f03   -O0  execution test



Paste the updated patch v3:


v3: Add compute_target_labels and call it in the front of make_blocks_1.

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 (stmt_starts_bb_p): Check whether the label is in
	target_labels.
	(compute_target_labels): New function.
	(make_blocks_1): Call compute_target_labels.

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/tree-cfg.cc                             | 68 ++++++++++++++++++++-
  gcc/testsuite/g++.dg/gcov/gcov-1.C          |  2 +-
  gcc/testsuite/gcc.dg/analyzer/paths-4.c     |  8 +--
  gcc/testsuite/gcc.misc-tests/gcov-pr85332.c |  2 +-
  gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++
  gcc/testsuite/lib/gcov.exp                  |  4 +-
  6 files changed, 96 insertions(+), 12 deletions(-)
  create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
  

On Wed, 8 Mar 2023, Xionghu Luo wrote:

> 
> 
> On 2023/3/7 19:25, Richard Biener wrote:
> >>> 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.
> 
> Tried to add function compute_target_labels(not sure whether the function
> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
> create/removing blocks now, but I still have several questions:
> 
>  1. I used hash_set<tree> to save the target labels instead of bitmap, as
> labels
> are tree type value instead of block index so bitmap is not good for it since
> we don't have LABEL_DECL_UID now?

We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
hash_set<tree> vs. bitmap is somewhat arbitrary here.  The real cost is
the extra walk over all stmts.

>  2. Is the compute_target_labels still only for !optimize?  And if we compute
> the target labels before create bbs, it is unnessary to guard the first
> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> case already create new block for CASE_LABEL already.

OK.

>  3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> labels_eh?

I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
coverage already?

> PS1: The v3 patch will cause one test case fail:
> 
> Number of regressions in total: 1
> > FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
> > errors)
> 
> due to this exausting case has labels from L0 to L100001, they won't be
> optimized
> to a simple if-else expression like before...

Hmm, that's somewhat unexpected.

> 
> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
> so
> just skip these label...

Please investigate, we might be missing a corner case here.

> 
> +       case GIMPLE_GOTO:
> +#if 0
> +         if (!computed_goto_p (stmt))
> +           {
> +             tree dest = gimple_goto_dest (stmt);
> +             target_labels->add (dest);
> +           }
> +#endif
> +         break;
> 
> Change the #if 0 to #if 1 result in:
> 
> Number of regressions in total: 8
> > FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
> > FAIL: errors)
> > FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> > FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> > FAIL: gfortran.dg/bound_2.f90   -O0  execution test
> > FAIL: gfortran.dg/bound_7.f90   -O0  execution test
> > FAIL: gfortran.dg/char_result_14.f90   -O0  execution test
> > FAIL: gfortran.dg/pointer_array_1.f90   -O0  execution test
> > FAIL: gfortran.dg/select_type_15.f03   -O0  execution test
> 
> 
> 
> Paste the updated patch v3:

The gcov testcase adjustments look good, does the analyzer testcase
(missing in the changelog) get different CFG input?

Thanks,
Richard.

> 
> v3: Add compute_target_labels and call it in the front of make_blocks_1.
> 
> 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 (stmt_starts_bb_p): Check whether the label is in
> 	target_labels.
> 	(compute_target_labels): New function.
> 	(make_blocks_1): Call compute_target_labels.
> 
> 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/tree-cfg.cc                             | 68 ++++++++++++++++++++-
>  gcc/testsuite/g++.dg/gcov/gcov-1.C          |  2 +-
>  gcc/testsuite/gcc.dg/analyzer/paths-4.c     |  8 +--
>  gcc/testsuite/gcc.misc-tests/gcov-pr85332.c |  2 +-
>  gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++
>  gcc/testsuite/lib/gcov.exp                  |  4 +-
>  6 files changed, 96 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> 
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..0f8efcf4aa3 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge,
> basic_block);
>  static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
>  
>  /* Various helpers.  */
> -static inline bool stmt_starts_bb_p (gimple *, gimple *);
> +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
>  static int gimple_verify_flow_info (void);
>  static void gimple_make_forwarder_block (edge);
>  static gimple *first_non_label_stmt (basic_block);
> @@ -521,6 +521,59 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
>      gimple_call_set_ctrl_altering (stmt, false);
>  }
>  
> +/* Compute target labels to save useful labels.  */
> +static void
> +compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
> +{
> +  gimple *stmt = NULL;
> +  gimple_stmt_iterator j = gsi_start (seq);
> +
> +  while (!gsi_end_p (j))
> +  {
> +      stmt = gsi_stmt (j);
> +
> +      switch (gimple_code (stmt))
> +      {
> +	case GIMPLE_COND:
> +	  {
> +	    gcond *cstmt = as_a <gcond *> (stmt);
> +	    tree true_label = gimple_cond_true_label (cstmt);
> +	    tree false_label = gimple_cond_false_label (cstmt);
> +	    target_labels->add (true_label);
> +	    target_labels->add (false_label);
> +	  }
> +	  break;
> +	case GIMPLE_SWITCH:
> +	  {
> +	    gswitch *gstmt = as_a <gswitch *> (stmt);
> +	    size_t i, n = gimple_switch_num_labels (gstmt);
> +	    tree elt, label;
> +	    for (i = 0; i < n; i++)
> +	    {
> +	      elt = gimple_switch_label (gstmt, i);
> +	      label = CASE_LABEL (elt);
> +	      target_labels->add (label);
> +	    }
> +	  }
> +	  break;
> +	case GIMPLE_GOTO:
> +#if 0
> +	  if (!computed_goto_p (stmt))
> +	    {
> +	      tree dest = gimple_goto_dest (stmt);
> +	      target_labels->add (dest);
> +	    }
> +#endif
> +	  break;
> +
> +	default:
> +	  break;
> +      }
> +
> +      gsi_next (&j);
> +  }
> +}
> +
>  
>  /* Insert SEQ after BB and build a flowgraph.  */
>  
> @@ -532,6 +585,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>    gimple *prev_stmt = NULL;
>    bool start_new_block = true;
>    bool first_stmt_of_seq = true;
> +  hash_set<tree> target_labels;
> +
> +  if (!optimize)
> +    compute_target_labels (seq, &target_labels);
>  
>    while (!gsi_end_p (i))
>      {
> @@ -553,7 +610,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>        /* If the statement starts a new basic block or if we have determined
>  	 in a previous pass that we need to create a new block for STMT, do
>  	 so now.  */
> -      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
> +      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt,
> &target_labels))
>  	{
>  	  if (!first_stmt_of_seq)
>  	    gsi_split_seq_before (&i, &seq);
> @@ -2832,7 +2889,8 @@ simple_goto_p (gimple *t)
>     label.  */
>  
>  static inline bool
> -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
> +		  hash_set<tree> *target_labels)
>  {
>    if (stmt == NULL)
>      return false;
> @@ -2860,6 +2918,10 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
>  	      || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
>  	    return true;
>  +	  if (!optimize
> +	      && target_labels->contains (gimple_label_label (label_stmt)))
> +	    return true;
> +
>  	  cfg_stats.num_merged_labels++;
>  	  return false;
>  	}
> 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.dg/analyzer/paths-4.c
> b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> index b72e658739e..fdf33e68d0c 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> @@ -35,18 +35,18 @@ int test_2 (struct state *s)
>  	  do_stuff (s, 0);
>  	  break;
>  	case 1:
> -	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> +	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
>  	  do_stuff (s, 17);
>  	  break;
>  	case 2:
> -	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> +	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
>  	  do_stuff (s, 5);
>  	  break;
>  	case 3:
> -	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> +	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
>  	  return 42;
>  	case 4:
> -	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> +	  __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
>  	  return -3;
>      	}
>      }
> 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
>
  

On 2023/3/9 20:02, Richard Biener wrote:
> On Wed, 8 Mar 2023, Xionghu Luo wrote:
> 
>>
>>
>> On 2023/3/7 19:25, Richard Biener wrote:
>>>>> 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.
>>
>> Tried to add function compute_target_labels(not sure whether the function
>> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
>> create/removing blocks now, but I still have several questions:
>>
>>   1. I used hash_set<tree> to save the target labels instead of bitmap, as
>> labels
>> are tree type value instead of block index so bitmap is not good for it since
>> we don't have LABEL_DECL_UID now?
> 
> We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> hash_set<tree> vs. bitmap is somewhat arbitrary here.  The real cost is
> the extra walk over all stmts.
> 
>>   2. Is the compute_target_labels still only for !optimize?  And if we compute
>> the target labels before create bbs, it is unnessary to guard the first
>> cleanup_dead_labels under !optimize now, because the switch-case-do-while
>> case already create new block for CASE_LABEL already.
> 
> OK.
> 
>>   3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
>> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
>> labels_eh?
> 
> I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> coverage already?

Added in patch v4.

> 
>> PS1: The v3 patch will cause one test case fail:
>>
>> Number of regressions in total: 1
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
>>> errors)
>>
>> due to this exausting case has labels from L0 to L100001, they won't be
>> optimized
>> to a simple if-else expression like before...
> 
> Hmm, that's somewhat unexpected.

It could be fixed by not start a new block if two locus are on same line as the
labels are expanded by MACRO with same location info.  BTW, I found that two
UNKOWN_LOCATION variable may have different value but return true in same_line_p?

2: locus1 = 2147483670
3: locus2 = 2147483652
(gdb) pel locus1
{file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc300, sysp = false}
(gdb) pel locus2
{file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc4e0, sysp = false}
(gdb) p LOCATION_LOCUS (locus1)
$16 = 0
(gdb) p LOCATION_LOCUS (locus2)
$17 = 0

So fix the function like this?

@@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location *from, location_t locus2)
  {
    expanded_location to;

+  if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
+      && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
+    return false;
+
    if (locus1 == locus2)
      return true;

> 
>>
>> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
>> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
>> so
>> just skip these label...
> 
> Please investigate, we might be missing a corner case here.

Yes.  Take the case pointer_array_1.f90 as example, it has an UNUSED label "L.7"
with locus info in it, not sure why it exists even since .original.


   [pointer_array_1.f90:39:10] if (test.14 != 0) goto <D.4599>; els
e goto <D.4600>;
   <D.4599>:
   [pointer_array_1.f90:39:52] _gfortran_stop_numeric (3, 0);
   <D.4600>:
   parm.16 = {CLOBBER(eol)};
   [pointer_array_1.f90:39:52] L.7:         <= UNUSED label
   <D.4594>:
   [pointer_array_1.f90:39:52] L.3:
   atmp.0 = {CLOBBER(eol)};
   A.1 = {CLOBBER(eol)};
   atmp.5 = {CLOBBER(eol)};
   A.6 = {CLOBBER(eol)};
   d = {CLOBBER(eol)};
   [pointer_array_1.f90:41:14] return;

stmt_starts_bb_p will return true for L.7 as the prev_stmt "parm.16 = {CLOBBER(eol)};"
is not a label statement, then <D.4594> will also return true in stmt_starts_bb_p as
the label_stmt and prev_stmt are NOT on same line.

<bb 38> :
L.9:
L.8:
if (test.14 != 0) goto <L39>; else goto <L40>;

<bb 39> :
<L39>:
_gfortran_stop_numeric (3, 0);

<bb 40> :
<L40>:
parm.16 = {CLOBBER(eol)};

<bb 41> :           <= empty block
L.7:

<bb 42> :
<L42>:
L.3:
atmp.0 = {CLOBBER(eol)};
A.1 = {CLOBBER(eol)};
atmp.5 = {CLOBBER(eol)};
A.6 = {CLOBBER(eol)};
d = {CLOBBER(eol)};
return;


So I have to fix it with this: Return false to not start a new block when prev_stmt
is not a label_statement and target_labels not containing the current label:

static inline bool stmt_starts_bb_p(gimple *stmt, gimple *prev_stmt, hash_set<tree> *target_labels)
{
...
        if (glabel *plabel = safe_dyn_cast <glabel *> (prev_stmt))
         {
           ...
           cfg_stats.num_merged_labels++;
           return false;
         }
+      else if (!optimize
+              && !target_labels->contains (gimple_label_label (label_stmt)))  <= Fix gfortran.dg failure
+              && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)      <= Fix bootstrap failure of openacc.f90
+       return false;
        else
         return true;
...
}

Though this fix would caused stage3 bootstrap failure quite like the fortran falure::

/data/src/gcc/libgomp/openacc.f90:1058:36:

  1058 | subroutine acc_get_property_string_h (devicenum, devicetype, property, string)
       |                                    ^
Error: label ‘L.3’ in the middle of basic block 13


<bb 12> :
D.4377 = i > D.4374;
if (D.4377 != 0)
   goto <bb 14>; [INV]
else
   goto <bb 13>; [INV]

<bb 13> :
_10 = sptr.data;
_11 = sptr.offset;
_12 = i + _11;
_13 = sptr.span;
_14 = _12 * _13;
_15 = (sizetype) _14;
_16 = _10 + _15;
_17 = MEM[(character(kind=1) *)_16];
(*string)[i]{lb: 1 sz: 1} = _17;
L.3:                  <= UNUSED label.
i = i + 1;
goto <bb 12>; [INV]

<bb 14> :
sptr = {CLOBBER(eol)};

<bb 15> :
<L18>:
return;

<bb 1> :

> 
>>
>> +       case GIMPLE_GOTO:
>> +#if 0
>> +         if (!computed_goto_p (stmt))
>> +           {
>> +             tree dest = gimple_goto_dest (stmt);
>> +             target_labels->add (dest);
>> +           }
>> +#endif
>> +         break;
>>
>> Change the #if 0 to #if 1 result in:
>>
>> Number of regressions in total: 8
>>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
>>> FAIL: errors)
>>> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
>>> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
>>> FAIL: gfortran.dg/bound_2.f90   -O0  execution test
>>> FAIL: gfortran.dg/bound_7.f90   -O0  execution test
>>> FAIL: gfortran.dg/char_result_14.f90   -O0  execution test
>>> FAIL: gfortran.dg/pointer_array_1.f90   -O0  execution test
>>> FAIL: gfortran.dg/select_type_15.f03   -O0  execution test
>>
>>
>>
>> Paste the updated patch v3:
> 
> The gcov testcase adjustments look good, does the analyzer testcase
> (missing in the changelog) get different CFG input?
> 

Yes, it was different.  But the analyzer case recovered with the same_line_p check.


One thing to note is that gimple_set_bb would refresh Label uid in it,
so target_labels need also update after it to follow that change?


@@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
          codes.  */
        gimple_set_bb (stmt, bb);

+      if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+       target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));


Another thing is the v4 patch will caused strange failures on libgomp.fortran testsuites.
It only happens when running "make check" but doesn't fail on single run, and more strangely,
the RUNTESTFLAGS doesn't work when running, it runs all cases under libgomp.fortran instead
of vla1.f90, which makes it difficult to analyze the failures...

make check-fortran RUNTESTFLAGS="fortran.exp=vla1.f90 -v -v" -j8

single run success:
/data/./gcc/xgcc -B/data/./gcc/ -B/data/install/x86_64-linux-gnu/bin/ -B/data/install/x86_64-linux-gnu/lib/ -isystem /data/install/x86_64-linux-gnu/include -isystem /data/install/x86_64-linux-gnu/sys-include    /data/RocksDB_Docker/tgcc-master/libgomp/testsuite/libgomp.fortran/vla1.f90   -B/data/RocksDB_Docker/debug_master/x86_64-linux-gnu/./libgomp/ -B/data/x86_64-linux-gnu/./libgomp/.libs -I/data/x86_64-linux-gnu/./libgomp -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/../../include -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/.. -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -fopenmp -B/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/   -O0   -B/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -fintrinsic-modules-path=/data/x86_64-linux-gnu/./libgomp   -L/data/x86_64-linux-gnu/./libgomp/.libs -L/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/ -L/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -lgfortran -foffload=-lgfortran -lquadmath -lm  -o ./vla1.exe

FAIL: libgomp.fortran/appendix-a/a.4.1.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/collapse2.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/task2.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/udr13.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/udr3.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/udr4.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla1.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla2.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla3.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla4.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla5.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla6.f90   -O0  (test for excess errors)
FAIL: libgomp.fortran/vla8.f90   -O0  (test for excess errors)
FAIL: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable  -O0  (test for excess errors)
FAIL: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable  -O0  (test for excess errors)
UNRESOLVED: libgomp.fortran/appendix-a/a.4.1.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/collapse2.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/task2.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr13.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr3.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/udr4.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla1.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla2.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla3.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla4.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla5.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla6.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.fortran/vla8.f90   -O0  compilation failed to produce executable
UNRESOLVED: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable  -O0  compilation failed to produce
  executable
UNRESOLVED: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable  -O0  compilation failed to
produce executable


v4: Address comments.
  4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
  4.2. Fix failure of limit-caselabels.c (labels on same line),
  pointer_array_1.f90 (unused labels) etc.

v3: Add compute_target_labels and call it in the front of make_blocks_1.
v2: Check whether two locus are on same line.

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 (stmt_starts_bb_p): Check whether the label is in
	target_labels.
	(compute_target_labels): New function.
	(make_blocks_1): Call compute_target_labels.
	(same_line_p): Return false if two locus are both
	UNKOWN_LOCATION.

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/tree-cfg.cc                             | 91 ++++++++++++++++++++-
  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 +-
  6 files changed, 116 insertions(+), 9 deletions(-)
  create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c

diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..d7ce121713e 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge, basic_block);
  static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
  
  /* Various helpers.  */
-static inline bool stmt_starts_bb_p (gimple *, gimple *);
+static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
  static int gimple_verify_flow_info (void);
  static void gimple_make_forwarder_block (edge);
  static gimple *first_non_label_stmt (basic_block);
@@ -521,6 +521,68 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
      gimple_call_set_ctrl_altering (stmt, false);
  }
  
+/* Compute target labels to save useful labels.  */
+static void
+compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
+{
+  gimple *stmt = NULL;
+  gimple_stmt_iterator j = gsi_start (seq);
+
+  while (!gsi_end_p (j))
+  {
+      stmt = gsi_stmt (j);
+
+      switch (gimple_code (stmt))
+      {
+	case GIMPLE_COND:
+	  {
+	    gcond *cstmt = as_a <gcond *> (stmt);
+	    tree true_label = gimple_cond_true_label (cstmt);
+	    tree false_label = gimple_cond_false_label (cstmt);
+	    target_labels->add (true_label);
+	    target_labels->add (false_label);
+	  }
+	  break;
+	case GIMPLE_SWITCH:
+	  {
+	    gswitch *gstmt = as_a <gswitch *> (stmt);
+	    size_t i, n = gimple_switch_num_labels (gstmt);
+	    tree elt, label;
+	    for (i = 0; i < n; i++)
+	    {
+	      elt = gimple_switch_label (gstmt, i);
+	      label = CASE_LABEL (elt);
+	      target_labels->add (label);
+	    }
+	  }
+	  break;
+	case GIMPLE_GOTO:
+	  if (!computed_goto_p (stmt))
+	    {
+	      tree dest = gimple_goto_dest (stmt);
+	      target_labels->add (dest);
+	    }
+	  break;
+	case GIMPLE_ASM:
+	  {
+	    gasm *asm_stmt = as_a <gasm *> (stmt);
+	    int i, n = gimple_asm_nlabels (asm_stmt);
+	    for (i = 0; i < n; ++i)
+	    {
+	      tree cons = gimple_asm_label_op (asm_stmt, i);
+	      target_labels->add (cons);
+	    }
+	  }
+	  break;
+
+	default:
+	  break;
+      }
+
+      gsi_next (&j);
+  }
+}
+
  
  /* Insert SEQ after BB and build a flowgraph.  */
  
@@ -532,6 +594,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
    gimple *prev_stmt = NULL;
    bool start_new_block = true;
    bool first_stmt_of_seq = true;
+  hash_set<tree> target_labels;
+
+  if (!optimize)
+    compute_target_labels (seq, &target_labels);
  
    while (!gsi_end_p (i))
      {
@@ -553,7 +619,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
        /* If the statement starts a new basic block or if we have determined
  	 in a previous pass that we need to create a new block for STMT, do
  	 so now.  */
-      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
+      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt, &target_labels))
  	{
  	  if (!first_stmt_of_seq)
  	    gsi_split_seq_before (&i, &seq);
@@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
  	 codes.  */
        gimple_set_bb (stmt, bb);
  
+      if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
+	target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
+
        /* If STMT is a basic block terminator, set START_NEW_BLOCK for the
  	 next iteration.  */
        if (stmt_ends_bb_p (stmt))
@@ -2832,7 +2901,8 @@ simple_goto_p (gimple *t)
     label.  */
  
  static inline bool
-stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
+stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
+		  hash_set<tree> *target_labels)
  {
    if (stmt == NULL)
      return false;
@@ -2860,9 +2930,24 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
  	      || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
  	    return true;
  
+	  location_t prev_locus = gimple_location (plabel);
+	  location_t locus = gimple_location (label_stmt);
+	  expanded_location locus_e = expand_location (locus);
+
+	  if (!optimize
+	      && target_labels->contains (gimple_label_label (label_stmt))
+	      && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+		|| LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+	      && !same_line_p (locus, &locus_e, prev_locus))
+	    return true;
+
  	  cfg_stats.num_merged_labels++;
  	  return false;
  	}
+      else if (!optimize
+	       && !target_labels->contains (gimple_label_label (label_stmt))
+	       && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
+	return false;
        else
  	return true;
      }
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
-- 
2.27.0
  

On Tue, 14 Mar 2023, Xionghu Luo wrote:

> 
> 
> On 2023/3/9 20:02, Richard Biener wrote:
> > On Wed, 8 Mar 2023, Xionghu Luo wrote:
> > 
> >>
> >>
> >> On 2023/3/7 19:25, Richard Biener wrote:
> >>>>> 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.
> >>
> >> Tried to add function compute_target_labels(not sure whether the function
> >> name is suitable) in the front of make_blocks_1, now the fortran case
> >> doesn't
> >> create/removing blocks now, but I still have several questions:
> >>
> >>   1. I used hash_set<tree> to save the target labels instead of bitmap, as
> >> labels
> >> are tree type value instead of block index so bitmap is not good for it
> >> since
> >> we don't have LABEL_DECL_UID now?
> > 
> > We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
> > hash_set<tree> vs. bitmap is somewhat arbitrary here.  The real cost is
> > the extra walk over all stmts.
> > 
> >>   2. Is the compute_target_labels still only for !optimize?  And if we
> >> compute
> >> the target labels before create bbs, it is unnessary to guard the first
> >> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> >> case already create new block for CASE_LABEL already.
> > 
> > OK.
> > 
> >>   3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> >> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> >> labels_eh?
> > 
> > I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
> > coverage already?
> 
> Added in patch v4.
> 
> > 
> >> PS1: The v3 patch will cause one test case fail:
> >>
> >> Number of regressions in total: 1
> >>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
> >>> errors)
> >>
> >> due to this exausting case has labels from L0 to L100001, they won't be
> >> optimized
> >> to a simple if-else expression like before...
> > 
> > Hmm, that's somewhat unexpected.
> 
> It could be fixed by not start a new block if two locus are on same line as
> the
> labels are expanded by MACRO with same location info.  BTW, I found that two
> UNKOWN_LOCATION variable may have different value but return true in
> same_line_p?

Yes, the raw location value also encodes other info so only
LOCATION_LOCUS (loc) will be equal to UNKNOWN_LOCATION.  There's some
existing inconsistency in whether LOCATION_LOCUS or raw locus is
compared.

> 2: locus1 = 2147483670
> 3: locus2 = 2147483652
> (gdb) pel locus1
> {file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc300, sysp = false}
> (gdb) pel locus2
> {file = 0x0, line = 0, column = 0, data = 0x7ffff6bdc4e0, sysp = false}
> (gdb) p LOCATION_LOCUS (locus1)
> $16 = 0
> (gdb) p LOCATION_LOCUS (locus2)
> $17 = 0
> 
> So fix the function like this?
> 
> @@ -1152,6 +1218,10 @@ same_line_p (location_t locus1, expanded_location
> *from, location_t locus2)
>  {
>    expanded_location to;
> 
> +  if (LOCATION_LOCUS (locus1) == UNKNOWN_LOCATION
> +      && LOCATION_LOCUS (locus2) == UNKNOWN_LOCATION)
> +    return false;
> +

I think we want to treat two unknown locations as the same line, but for
consistency I'd change the following test to use LOCATION_LOCUS

>    if (locus1 == locus2)
>      return true;
> 
> > 
> >>
> >> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail
> >> due
> >> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into
> >> it
> >> so
> >> just skip these label...
> > 
> > Please investigate, we might be missing a corner case here.
> 
> Yes.  Take the case pointer_array_1.f90 as example, it has an UNUSED label
> "L.7"
> with locus info in it, not sure why it exists even since .original.
> 
> 
>   [pointer_array_1.f90:39:10] if (test.14 != 0) goto <D.4599>; els
> e goto <D.4600>;
>   <D.4599>:
>   [pointer_array_1.f90:39:52] _gfortran_stop_numeric (3, 0);
>   <D.4600>:
>   parm.16 = {CLOBBER(eol)};
>   [pointer_array_1.f90:39:52] L.7:         <= UNUSED label
>   <D.4594>:
>   [pointer_array_1.f90:39:52] L.3:
>   atmp.0 = {CLOBBER(eol)};
>   A.1 = {CLOBBER(eol)};
>   atmp.5 = {CLOBBER(eol)};
>   A.6 = {CLOBBER(eol)};
>   d = {CLOBBER(eol)};
>   [pointer_array_1.f90:41:14] return;
> 
> stmt_starts_bb_p will return true for L.7 as the prev_stmt "parm.16 =
> {CLOBBER(eol)};"
> is not a label statement, then <D.4594> will also return true in
> stmt_starts_bb_p as
> the label_stmt and prev_stmt are NOT on same line.
> 
> <bb 38> :
> L.9:
> L.8:
> if (test.14 != 0) goto <L39>; else goto <L40>;
> 
> <bb 39> :
> <L39>:
> _gfortran_stop_numeric (3, 0);
> 
> <bb 40> :
> <L40>:
> parm.16 = {CLOBBER(eol)};
> 
> <bb 41> :           <= empty block
> L.7:

So this is another case that's fixed by the make_edges_bb fix (I
see you followed up this mail with something similar).

Please use my original suggested fix though, do

 if (!last)
    fallthru = true;

  else
    switch (gimple_code (last))
      {
...

instead of the if (!last) return ret, that should be done independent
of !optimize

> <bb 42> :
> <L42>:
> L.3:
> atmp.0 = {CLOBBER(eol)};
> A.1 = {CLOBBER(eol)};
> atmp.5 = {CLOBBER(eol)};
> A.6 = {CLOBBER(eol)};
> d = {CLOBBER(eol)};
> return;
> 
> 
> So I have to fix it with this: Return false to not start a new block when
> prev_stmt
> is not a label_statement and target_labels not containing the current label:
> 
> static inline bool stmt_starts_bb_p(gimple *stmt, gimple *prev_stmt,
> hash_set<tree> *target_labels)
> {
> ...
>        if (glabel *plabel = safe_dyn_cast <glabel *> (prev_stmt))
>         {
>           ...
>           cfg_stats.num_merged_labels++;
>           return false;
>         }
> +      else if (!optimize
> +              && !target_labels->contains (gimple_label_label (label_stmt)))
> <= Fix gfortran.dg failure
> +              && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
> <= Fix bootstrap failure of openacc.f90
> +       return false;
>        else
>         return true;
> ...
> }
> 
> Though this fix would caused stage3 bootstrap failure quite like the fortran
> falure::
> 
> /data/src/gcc/libgomp/openacc.f90:1058:36:
> 
>  1058 | subroutine acc_get_property_string_h (devicenum, devicetype, property,
> string)
>       |                                    ^
> Error: label ‘L.3’ in the middle of basic block 13
> 
> 
> <bb 12> :
> D.4377 = i > D.4374;
> if (D.4377 != 0)
>   goto <bb 14>; [INV]
> else
>   goto <bb 13>; [INV]
> 
> <bb 13> :
> _10 = sptr.data;
> _11 = sptr.offset;
> _12 = i + _11;
> _13 = sptr.span;
> _14 = _12 * _13;
> _15 = (sizetype) _14;
> _16 = _10 + _15;
> _17 = MEM[(character(kind=1) *)_16];
> (*string)[i]{lb: 1 sz: 1} = _17;
> L.3:                  <= UNUSED label.
> i = i + 1;
> goto <bb 12>; [INV]
> 
> <bb 14> :
> sptr = {CLOBBER(eol)};
> 
> <bb 15> :
> <L18>:
> return;
> 
> <bb 1> :
> 
> > 
> >>
> >> +       case GIMPLE_GOTO:
> >> +#if 0
> >> +         if (!computed_goto_p (stmt))
> >> +           {
> >> +             tree dest = gimple_goto_dest (stmt);
> >> +             target_labels->add (dest);
> >> +           }
> >> +#endif
> >> +         break;
> >>
> >> Change the #if 0 to #if 1 result in:
> >>
> >> Number of regressions in total: 8
> >>> FAIL: gcc.c-torture/compile/limits-caselabels.c   -O0  (test for excess
> >>> FAIL: errors)
> >>> FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> >>> FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> >>> FAIL: gfortran.dg/bound_2.f90   -O0  execution test
> >>> FAIL: gfortran.dg/bound_7.f90   -O0  execution test
> >>> FAIL: gfortran.dg/char_result_14.f90   -O0  execution test
> >>> FAIL: gfortran.dg/pointer_array_1.f90   -O0  execution test
> >>> FAIL: gfortran.dg/select_type_15.f03   -O0  execution test
> >>
> >>
> >>
> >> Paste the updated patch v3:
> > 
> > The gcov testcase adjustments look good, does the analyzer testcase
> > (missing in the changelog) get different CFG input?
> > 
> 
> Yes, it was different.  But the analyzer case recovered with the same_line_p
> check.
> 
> 
> One thing to note is that gimple_set_bb would refresh Label uid in it,
> so target_labels need also update after it to follow that change?
> 
> 
> @@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>          codes.  */
>        gimple_set_bb (stmt, bb);
> 
> +      if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
> +       target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
> 
> 
> Another thing is the v4 patch will caused strange failures on libgomp.fortran
> testsuites.
> It only happens when running "make check" but doesn't fail on single run, and
> more strangely,
> the RUNTESTFLAGS doesn't work when running, it runs all cases under
> libgomp.fortran instead
> of vla1.f90, which makes it difficult to analyze the failures...
> 
> make check-fortran RUNTESTFLAGS="fortran.exp=vla1.f90 -v -v" -j8
> 
> single run success:
> /data/./gcc/xgcc -B/data/./gcc/ -B/data/install/x86_64-linux-gnu/bin/
> -B/data/install/x86_64-linux-gnu/lib/ -isystem
> /data/install/x86_64-linux-gnu/include -isystem
> /data/install/x86_64-linux-gnu/sys-include
> /data/RocksDB_Docker/tgcc-master/libgomp/testsuite/libgomp.fortran/vla1.f90
> -B/data/RocksDB_Docker/debug_master/x86_64-linux-gnu/./libgomp/
> -B/data/x86_64-linux-gnu/./libgomp/.libs -I/data/x86_64-linux-gnu/./libgomp
> -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/../../include
> -I/data/RocksDB_Docker/tgcc-master/libgomp/testsuite/.. -fmessage-length=0
> -fno-diagnostics-show-caret -fdiagnostics-color=never -fopenmp
> -B/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/   -O0
> -B/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs
> -fintrinsic-modules-path=/data/x86_64-linux-gnu/./libgomp
> -L/data/x86_64-linux-gnu/./libgomp/.libs
> -L/data/x86_64-linux-gnu/./libgomp/../libquadmath/.libs/
> -L/data/x86_64-linux-gnu/./libgomp/../libgfortran/.libs -lgfortran
> -foffload=-lgfortran -lquadmath -lm  -o ./vla1.exe
> 
> FAIL: libgomp.fortran/appendix-a/a.4.1.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/collapse2.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/task2.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/udr13.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/udr3.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/udr4.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla1.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla2.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla3.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla4.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla5.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla6.f90   -O0  (test for excess errors)
> FAIL: libgomp.fortran/vla8.f90   -O0  (test for excess errors)
> FAIL: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1
> FAIL: -DACC_MEM_SHARED=1 -foffload=disable  -O0  (test for excess errors)
> FAIL: libgomp.oacc-fortran/nested-function-1.f90 -DACC_DEVICE_TYPE_host=1
> FAIL: -DACC_MEM_SHARED=1 -foffload=disable  -O0  (test for excess errors)
> UNRESOLVED: libgomp.fortran/appendix-a/a.4.1.f90   -O0  compilation failed to
> UNRESOLVED: produce executable
> UNRESOLVED: libgomp.fortran/collapse2.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/task2.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr13.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr3.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/udr4.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla1.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla2.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla3.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla4.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla5.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla6.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.fortran/vla8.f90   -O0  compilation failed to produce
> UNRESOLVED: executable
> UNRESOLVED: libgomp.oacc-fortran/collapse-2.f90 -DACC_DEVICE_TYPE_host=1
> UNRESOLVED: -DACC_MEM_SHARED=1 -foffload=disable  -O0  compilation failed to
> UNRESOLVED: produce
>  executable
> UNRESOLVED: libgomp.oacc-fortran/nested-function-1.f90
> -DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 -foffload=disable  -O0
> compilation failed to
> produce executable
> 
> 
> v4: Address comments.
>  4.1. Handle GIMPLE_GOTO and GIMPLE_ASM.
>  4.2. Fix failure of limit-caselabels.c (labels on same line),
>  pointer_array_1.f90 (unused labels) etc.
> 
> v3: Add compute_target_labels and call it in the front of make_blocks_1.
> v2: Check whether two locus are on same line.
> 
> 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?

Note this has to wait for stage1 to open I think.

Some comments on the patch below

> gcc/ChangeLog:
> 
> 	PR gcov/93680
> 	* tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
> 	target_labels.
> 	(compute_target_labels): New function.
> 	(make_blocks_1): Call compute_target_labels.
> 	(same_line_p): Return false if two locus are both
> 	UNKOWN_LOCATION.
> 
> 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/tree-cfg.cc                             | 91 ++++++++++++++++++++-
>  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 +-
>  6 files changed, 116 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> 
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..d7ce121713e 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge,
> basic_block);
>  static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
>  
>  /* Various helpers.  */
> -static inline bool stmt_starts_bb_p (gimple *, gimple *);
> +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
>  static int gimple_verify_flow_info (void);
>  static void gimple_make_forwarder_block (edge);
>  static gimple *first_non_label_stmt (basic_block);
> @@ -521,6 +521,68 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
>      gimple_call_set_ctrl_altering (stmt, false);
>  }
>  
> +/* Compute target labels to save useful labels.  */
> +static void
> +compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
> +{
> +  gimple *stmt = NULL;
> +  gimple_stmt_iterator j = gsi_start (seq);
> +
> +  while (!gsi_end_p (j))
> +  {
> +      stmt = gsi_stmt (j);
> +
> +      switch (gimple_code (stmt))
> +      {
> +	case GIMPLE_COND:
> +	  {
> +	    gcond *cstmt = as_a <gcond *> (stmt);
> +	    tree true_label = gimple_cond_true_label (cstmt);
> +	    tree false_label = gimple_cond_false_label (cstmt);
> +	    target_labels->add (true_label);
> +	    target_labels->add (false_label);
> +	  }
> +	  break;
> +	case GIMPLE_SWITCH:
> +	  {
> +	    gswitch *gstmt = as_a <gswitch *> (stmt);
> +	    size_t i, n = gimple_switch_num_labels (gstmt);
> +	    tree elt, label;
> +	    for (i = 0; i < n; i++)
> +	    {
> +	      elt = gimple_switch_label (gstmt, i);
> +	      label = CASE_LABEL (elt);
> +	      target_labels->add (label);
> +	    }
> +	  }
> +	  break;
> +	case GIMPLE_GOTO:
> +	  if (!computed_goto_p (stmt))
> +	    {
> +	      tree dest = gimple_goto_dest (stmt);
> +	      target_labels->add (dest);
> +	    }
> +	  break;
> +	case GIMPLE_ASM:
> +	  {
> +	    gasm *asm_stmt = as_a <gasm *> (stmt);
> +	    int i, n = gimple_asm_nlabels (asm_stmt);
> +	    for (i = 0; i < n; ++i)
> +	    {
> +	      tree cons = gimple_asm_label_op (asm_stmt, i);
> +	      target_labels->add (cons);
> +	    }
> +	  }
> +	  break;
> +
> +	default:
> +	  break;
> +      }
> +
> +      gsi_next (&j);
> +  }
> +}
> +
>  
>  /* Insert SEQ after BB and build a flowgraph.  */
>  
> @@ -532,6 +594,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>    gimple *prev_stmt = NULL;
>    bool start_new_block = true;
>    bool first_stmt_of_seq = true;
> +  hash_set<tree> target_labels;
> +
> +  if (!optimize)
> +    compute_target_labels (seq, &target_labels);
>  
>    while (!gsi_end_p (i))
>      {
> @@ -553,7 +619,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>        /* If the statement starts a new basic block or if we have determined
>  	 in a previous pass that we need to create a new block for STMT, do
>  	 so now.  */
> -      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
> +      if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt,
> &target_labels))

quoting your stmt_starts_bb_p change:

+         location_t prev_locus = gimple_location (plabel);
+         location_t locus = gimple_location (label_stmt);
+         expanded_location locus_e = expand_location (locus);
+
+         if (!optimize
+             && target_labels->contains (gimple_label_label (label_stmt))
+             && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
+               || LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
+             && !same_line_p (locus, &locus_e, prev_locus))
+           return true;

I think the UNKNOWN_LOCATION check isn't necessary?  Two unknown
location locs will compare equal (and I think they should).

>  	{
>  	  if (!first_stmt_of_seq)
>  	    gsi_split_seq_before (&i, &seq);
> @@ -566,6 +632,9 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
>        	 codes.  */
>        gimple_set_bb (stmt, bb);
>  +      if (!optimize && gimple_code (stmt) == GIMPLE_LABEL)
> +	target_labels.add (gimple_label_label (as_a<glabel *> (stmt)));
> +

Huh, why?  We're never going to visit the stmt again and thus never
look up the label again?

>        /* If STMT is a basic block terminator, set START_NEW_BLOCK for the
>        	 next iteration.  */
>        if (stmt_ends_bb_p (stmt))
> @@ -2832,7 +2901,8 @@ simple_goto_p (gimple *t)
>     label.  */
>  
>  static inline bool
> -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
> +		  hash_set<tree> *target_labels)
>  {
>    if (stmt == NULL)
>      return false;
> @@ -2860,9 +2930,24 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
>  	      || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
>  	    return true;
>  +	  location_t prev_locus = gimple_location (plabel);
> +	  location_t locus = gimple_location (label_stmt);
> +	  expanded_location locus_e = expand_location (locus);
> +
> +	  if (!optimize
> +	      && target_labels->contains (gimple_label_label (label_stmt))
> +	      && (LOCATION_LOCUS (locus) != UNKNOWN_LOCATION
> +		|| LOCATION_LOCUS (prev_locus) != UNKNOWN_LOCATION)
> +	      && !same_line_p (locus, &locus_e, prev_locus))
> +	    return true;
> +
>  	  cfg_stats.num_merged_labels++;
>  	  return false;
>  	}
> +      else if (!optimize
> +	       && !target_labels->contains (gimple_label_label (label_stmt))
> +	       && stmt->next && gimple_code (stmt->next) == GIMPLE_LABEL)
> +	return false;

Huh?  That would leave a label after a non-label stmt which is invalid.

Thanks and sorry for the delay.

Richard.

>        else
>      	return true;
>      }
> 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
>