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

[Xionghu Luo <yinyuefengyi@gmail.com>]

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

>> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
>> of gfortran cases execution failure with O0.  Take gfortran.dg/index.f90 for
>> example:
>>
>> .gimple:
>>
>> __attribute__((fn spec (". ")))
>> void p ()
>> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
>>    L.1:
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
>>    L.2:
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
>>    L.3:
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
>>    L.4:
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
>>    L.5:
>>    [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
>>    L.6:
>> }
>>
>> .cfg:
>>
>> ...
>> Removing basic block 7
>> ;; basic block 7, loop depth 0
>> ;;  pred:
>> return;
>> ;;  succ:       EXIT
>>
>>
>> ;; 1 loops found
>> ;;
>> ;; Loop 0
>> ;;  header 0, latch 1
>> ;;  depth 0, outer -1
>> ;;  nodes: 0 1 2
>> ;;2 succs { }
>> __attribute__((fn spec (". ")))
>> void p ()
>> {
>>    <bb 2> :
>>
>> }
>>
>> Due to the "return;" is removed in bb 7.
> 
> OK, the issue is that make_edges_bb does nothing for an empty block
> but it should at least create a fallthru edge here.  Thus,
> 
>    if (!last)
>      fallthru = true;
> 
>    else
>      switch (gimple_code (last))
>        {
> 
> instead of simply returning if (!last).  The alternative would be
> to make sure that cleanup_dead_labels preserves at least one
> statement in a block.
> 
> Looking at the testcases I wonder if preserving all the fallthru labels
> is really necessary - for coverage we should have a counter ready.  For
> the testcase we arrive with
> 
> L.1:
> L.2:
> L.3:
> L.4:
> i = 1;

It was:

<bb 0> :

<bb 2> :
L.1:

<bb 3> :
L.2:

<bb 4> :
L.3:

<bb 5> :
L.4:

<bb 6> :
L.5:

<bb 7> :
L.6:
return;

<bb 1> :

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

> 
> where the frontend simplified things but put labels at each line.
> I suppose we could optimize this by re-computing TREE_USED and only
> splitting before labels reached by a control statement?  That would
> cover the backedge case in the original testcase.  cleanup_dead_labels
> does something like that already.
> 
>> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1
>> to L.6
>> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they
>> are all
>> EMPTY bb in make_edges_bb...
>>   
>>
>>    240│   /* To speed up statement iterator walks, we first purge dead labels.
>>    */
>>    241│   cleanup_dead_labels ();
>>    242│
>>    243│   /* Group case nodes to reduce the number of edges.
>>    244│      We do this after cleaning up dead labels because otherwise we
>>    miss
>>    245│      a lot of obvious case merging opportunities.  */
>>    246│   group_case_labels ();
>>    247│
>>    248│   /* Create the edges of the flowgraph.  */
>>    249│   discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
>>    250├>  make_edges ();
>>
>>
>> <bb 0> :
>>
>> <bb 2> :
>>
>> <bb 3> :
>>
>> <bb 4> :
>>
>> <bb 5> :
>>
>> <bb 6> :
>>
>> <bb 7> :
>> return;
>>
>> <bb 1> :
>>
>>
>> Seems deadlock here as you said to set goto_locus as labels are removed before
>> edges are created, the case could pass if I comment out the function
>> cleanup_dead_labels(),
>> so also not call it when !optimize?
>>
>> if (!!optimize)
>>   cleanup_dead_labels ();
> 
> That probably makes sense.  Looking at group_case_labels () that also
> seems to do unwanted things (to debugging and coverage), its comment
> says that for
> 
>   switch (i)
>   {
>   case 1:
>     /* fallthru */
>   case 2:
>     /* fallthru */
>   case 3:
>     k = 0;
> 
> it would replace that with
> 
>   case 1..3:
>     k = 0;
> 
> but that also fails to produce correct coverage, right?  Likewise
> setting breakpoints.

Yes.  Should also exclude this.

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

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

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


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

> 
> It would be nice to avoid creating blocks / preserving labels we'll
> immediately remove again.  For that we do need some analysis
> before creating basic-blocks that determines whether a label is
> possibly reached by a non-falltru edge.
> 


<bb 2> :
p = 0;
switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>

<bb 3> :
<L0>:           <= prev_stmt
<D.2748>:       <= stmt
p = p + 1;
n = n + -1;
if (n != 0) goto <D.2748>; else goto <D.2746>;

Check if <L0> is a case label and <D.2748> is a goto target then return true
in stmt_starts_bb_p to start a new basic block?  This would avoid creating and
removing blocks, but cleanup_dead_labels has all bbs setup while stmt_starts_bb_p
does't yet to iterate bbs/labels to establish label_for_bb[] map?


Thanks,
Xionghu