On 2023/3/6 16:11, Richard Biener wrote: > On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo 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 >>> >>> : >>> 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)) [INV], [t.c:7:5] case 0: >>> [INV], [t.c:11:5] case 1: [INV]> >>> >>> : >>> # n_1 = PHI >>> # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)> >>> [t.c:7:5] : >>> [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 ; [INV] >>> else >>> goto ; [INV] >>> >>> : >>> 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 ; [100.00%] >>> >>> : >>> 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 ; [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 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) , [small.c:7:5] case 0: , [small.c:11:5] case 1: > >> [small.c:7:5] : <= case label >> : <= loop label >> [small.c:8:13] p = p + 1; >> [small.c:8:26] n = n + -1; >> [small.c:8:26] if (n != 0) goto ; else goto ; >> : >> [small.c:9:14] D.2755 = p; >> [small.c:9:14] return D.2755; >> [small.c:11:5] : >> : >> [small.c:12:13] p = p + 1; >> [small.c:12:26] n = n + -1; >> [small.c:12:26] if (n != 0) goto ; else goto ; >> : >> [small.c:13:14] D.2755 = p; >> [small.c:13:14] return D.2755; >> : >> [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; >> >> : >> [small.c:3:7] p = 0; >> [small.c:5:3] switch (s) [INV], [small.c:7:5] case 0: [INV], [small.c:11:5] case 1: [INV]> >> >> : >> [small.c:7:5] : <= 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 ; [INV] >> else >> goto ; [INV] >> >> : >> [small.c:9:14] D.2755 = p; >> [small.c:9:14] goto ; [INV] >> >> : >> [small.c:11:5] : <= 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 ; [INV] >> else >> goto ; [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 (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 () { : } Due to the "return;" is removed in bb 7. 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 to 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 (13); 250├> make_edges (); : : : : : : : return; : 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 (); Attached v2 patch could pass regression test on x86_64-linux-gnu/aarch64-linux-gnu.