Re: [PATCH 1/2] 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: 12929 bytes --]

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

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