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: 7467 bytes --]

On Tue, 7 Mar 2023, Xionghu Luo wrote:

> 
> 
> On 2023/3/7 16:53, Richard Biener wrote:
> > On Tue, 7 Mar 2023, Xionghu Luo wrote:
> 
> >> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
> >> of gfortran cases execution failure with O0.  Take gfortran.dg/index.f90
> >> for
> >> example:
> >>
> >> .gimple:
> >>
> >> __attribute__((fn spec (". ")))
> >> void p ()
> >> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9]
> >> {
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
> >>    L.1:
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
> >>    L.2:
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
> >>    L.3:
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
> >>    L.4:
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
> >>    L.5:
> >>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
> >>    L.6:
> >> }
> >>
> >> .cfg:
> >>
> >> ...
> >> Removing basic block 7
> >> ;; basic block 7, loop depth 0
> >> ;;  pred:
> >> return;
> >> ;;  succ:       EXIT
> >>
> >>
> >> ;; 1 loops found
> >> ;;
> >> ;; Loop 0
> >> ;;  header 0, latch 1
> >> ;;  depth 0, outer -1
> >> ;;  nodes: 0 1 2
> >> ;;2 succs { }
> >> __attribute__((fn spec (". ")))
> >> void p ()
> >> {
> >>    <bb 2> :
> >>
> >> }
> >>
> >> Due to the "return;" is removed in bb 7.
> > 
> > OK, the issue is that make_edges_bb does nothing for an empty block
> > but it should at least create a fallthru edge here.  Thus,
> > 
> >    if (!last)
> >      fallthru = true;
> > 
> >    else
> >      switch (gimple_code (last))
> >        {
> > 
> > instead of simply returning if (!last).  The alternative would be
> > to make sure that cleanup_dead_labels preserves at least one
> > statement in a block.
> > 
> > Looking at the testcases I wonder if preserving all the fallthru labels
> > is really necessary - for coverage we should have a counter ready.  For
> > the testcase we arrive with
> > 
> > L.1:
> > L.2:
> > L.3:
> > L.4:
> > i = 1;
> 
> It was:
> 
> <bb 0> :
> 
> <bb 2> :
> L.1:
> 
> <bb 3> :
> L.2:
> 
> <bb 4> :
> L.3:
> 
> <bb 5> :
> L.4:
> 
> <bb 6> :
> L.5:
> 
> <bb 7> :
> L.6:
> return;
> 
> <bb 1> :
> 
> before the second call of cleanup_dead_labels, after it, all labels are
> removed, then tree_forwarder_block_p remove all forworders.  Yes, it
> creates blocks and remove blocks immediately...
> 
> > 
> > where the frontend simplified things but put labels at each line.
> > I suppose we could optimize this by re-computing TREE_USED and only
> > splitting before labels reached by a control statement?  That would
> > cover the backedge case in the original testcase.  cleanup_dead_labels
> > does something like that already.
> > 
> >> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels
> >> L.1
> >> to L.6
> >> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to
> >> they
> >> are all
> >> EMPTY bb in make_edges_bb...
> >>   
> >>
> >>    240│   /* To speed up statement iterator walks, we first purge dead
> >>    labels.
> >>    */
> >>    241│   cleanup_dead_labels ();
> >>    242│
> >>    243│   /* Group case nodes to reduce the number of edges.
> >>    244│      We do this after cleaning up dead labels because otherwise we
> >>    miss
> >>    245│      a lot of obvious case merging opportunities.  */
> >>    246│   group_case_labels ();
> >>    247│
> >>    248│   /* Create the edges of the flowgraph.  */
> >>    249│   discriminator_per_locus = new hash_table<locus_discrim_hasher>
> >>    (13);
> >>    250├>  make_edges ();
> >>
> >>
> >> <bb 0> :
> >>
> >> <bb 2> :
> >>
> >> <bb 3> :
> >>
> >> <bb 4> :
> >>
> >> <bb 5> :
> >>
> >> <bb 6> :
> >>
> >> <bb 7> :
> >> return;
> >>
> >> <bb 1> :
> >>
> >>
> >> Seems deadlock here as you said to set goto_locus as labels are removed
> >> before
> >> edges are created, the case could pass if I comment out the function
> >> cleanup_dead_labels(),
> >> so also not call it when !optimize?
> >>
> >> if (!!optimize)
> >>   cleanup_dead_labels ();
> > 
> > That probably makes sense.  Looking at group_case_labels () that also
> > seems to do unwanted things (to debugging and coverage), its comment
> > says that for
> > 
> >   switch (i)
> >   {
> >   case 1:
> >     /* fallthru */
> >   case 2:
> >     /* fallthru */
> >   case 3:
> >     k = 0;
> > 
> > it would replace that with
> > 
> >   case 1..3:
> >     k = 0;
> > 
> > but that also fails to produce correct coverage, right?  Likewise
> > setting breakpoints.
> 
> Yes.  Should also exclude this.
> 
> > 
> > Does preserving the labels help setting a goto_locus for the
> > fallthru edges?  I don't see any code doing that, so
> > CFG cleanup will remove the forwarders we created again.
> 
> For the backedge case with switch-case-do-while, tree_forwarder_block_p
> returns false when iterating the statement check.
> The new created <bb 3> with only one case label instruction still owns
> location information in it, so CFG cleanup won't remove the forwarders.
> 
>  390│   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
>  391│     {
>  392│       gimple *stmt = gsi_stmt (gsi);
>  393│
>  394│       switch (gimple_code (stmt))
>  395│     {
>  396│     case GIMPLE_LABEL:
>  397│       if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
>  398│         return false;
>  399│       if (!optimize
>  400│           && (gimple_has_location (stmt)
>  401│           || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
>  402│           && gimple_location (stmt) != locus)
>  403├>        return false;
>  404│       break;
> 
> 
> (gdb) ps stmt
> <L0>:
> (gdb) p gimple_location (stmt)
> $154 = 2147483656
> (gdb) pel $154
> {file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp
> = false}
> (gdb)
> (gdb) pbb bb
> ;; basic block 3, loop depth 0
> ;;  pred:       2
> <L0>:
> ;;  succ:       4
> 
> > 
> > It would be nice to avoid creating blocks / preserving labels we'll
> > immediately remove again.  For that we do need some analysis
> > before creating basic-blocks that determines whether a label is
> > possibly reached by a non-falltru edge.
> > 
> 
> 
> <bb 2> :
> p = 0;
> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
> 
> <bb 3> :
> <L0>:           <= prev_stmt
> <D.2748>:       <= stmt
> p = p + 1;
> n = n + -1;
> if (n != 0) goto <D.2748>; else goto <D.2746>;
> 
> Check if <L0> is a case label and <D.2748> is a goto target then return true
> in stmt_starts_bb_p to start a new basic block?  This would avoid creating and
> removing blocks, but cleanup_dead_labels has all bbs setup while
> stmt_starts_bb_p
> does't yet to iterate bbs/labels to establish label_for_bb[] map?

Yes.  I think we'd need something more pragmatic before make_blocks (),
like re-computing TREE_USED of the label decls or computing a bitmap
of targeted labels (targeted by goto, switch or any other means).

I'll note that doing a cleanup_dead_labels () like optimization before
we create blocks will help keeping LABEL_DECL_UID and thus
label_to_block_map dense.  But it does look like a bit of 
an chicken-and-egg problem and the question is how effective the
dead label removal is in practice.

Richard.