On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefen...@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.
Thanks,
Richard.
> .profile:
>
> <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;
> [small.c:3:7] p_6 = 0;
> [small.c:5:3] switch (s_7(D)) <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
> PROF_edge_counter_19 = __gcov0.f[1];
> PROF_edge_counter_20 = PROF_edge_counter_19 + 1;
> __gcov0.f[1] = PROF_edge_counter_20;
>
> <bb 4> :
> # n_1 = PHI <n_8(D)(3), [small.c:8:26 discrim 1] n_13(5)>
> # p_3 = PHI <[small.c:3:7] p_6(3), [small.c:8:13 discrim 1] p_12(5)>
> [small.c:8:13 discrim 1] p_12 = p_3 + 1;
> [small.c:8:26 discrim 1] n_13 = n_1 + -1;
> [small.c:8:26 discrim 1] if (n_13 != 0)
> goto <bb 5>; [INV]
> else
> goto <bb 6>; [INV]
>
> <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;
> goto <bb 4>; [100.00%]
>
> <bb 6> :
> [small.c:9:14] _14 = p_12;
> [small.c:9:14] goto <bb 12>; [INV]
>
> <bb 7> :
> [small.c:11:5] <L3>: <= case 1
> PROF_edge_counter_21 = __gcov0.f[2];
> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> __gcov0.f[2] = PROF_edge_counter_22;
>
>
> <bb 8> :
> # n_2 = PHI <n_8(D)(7), [small.c:12:26 discrim 1] n_10(9)>
> # p_4 = PHI <[small.c:3:7] p_6(7), [small.c:12:13 discrim 1] p_9(9)>
> [small.c:12:13 discrim 1] p_9 = p_4 + 1;
> [small.c:12:26 discrim 1] n_10 = n_2 + -1;
> [small.c:12:26 discrim 1] if (n_10 != 0)
> goto <bb 9>; [INV]
> else
> goto <bb 10>; [INV]
>
> <bb 9> :
> PROF_edge_counter_25 = __gcov0.f[4];
> PROF_edge_counter_26 = PROF_edge_counter_25 + 1;
> __gcov0.f[4] = PROF_edge_counter_26;
> goto <bb 8>; [100.00%]
>
>
> then label lines are also correct now:
>
> .c.gcov:
>
> Lines executed:90.91% of 11
> -: 0:Source:small.c
> -: 0:Graph:small.gcno
> -: 0:Data:small.gcda
> -: 0:Runs:1
> 2: 1:int f(int s, int n)
> -: 2:{
> 2: 3: int p = 0;
> -: 4:
> 2: 5: switch (s)
> -: 6: {
> 1: 7: case 0:
> 5: 8: do { p++; } while (--n);
> 1: 9: return p;
> -: 10:
> 1: 11: case 1:
> 5: 12: do { p++; } while (--n);
> 1: 13: return p;
> -: 14: }
> -: 15:
> #####: 16: return 0;
> -: 17:}
> -: 18:
> 1: 19:int main() { f(0, 5); f(1, 5);}
