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

[Richard Biener <rguenther@suse.de>]

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

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
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)