From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x231.google.com (mail-lj1-x231.google.com [IPv6:2a00:1450:4864:20::231]) by sourceware.org (Postfix) with ESMTPS id A8E2E3858D35; Mon, 6 Mar 2023 08:12:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A8E2E3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-x231.google.com with SMTP id h9so8719675ljq.2; Mon, 06 Mar 2023 00:12:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678090322; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=vJM0yQUZriO4OnkmkcAwaVtSO0vUGjZMnCJ2fvZhq3E=; b=m2/XFY9a8bp7ISx6j6L7PkkfepyGJ338dVtiHqTaU4JKRp1ma1ZTt6liY2iud0pQIm aWJJ29mDUQQMp5l06wYxvOKhdUEfFXrPaOMDA5KdkDSVA2hev0eE8EPw3O7X11O/2zNm V7oOXByPlv3XeBj6D2UHpFSrcIRQqk63gaTIHAfePatjvQsYznXolBZa7jjLQam7IrdT 70F5MpsJmW49LMpfRMmZn/wtMOgNR0goujhDRgdlbgTbd5FcfCG2Gkdm43NBAqJhCGVV +7Zu7MdjCPuX0KUiIDiAollKc7TCbWfCbPyAEpHQUVNoA2hW2e2EB/CzMlfVNXvvFD3q Busw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678090322; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=vJM0yQUZriO4OnkmkcAwaVtSO0vUGjZMnCJ2fvZhq3E=; b=lZ2VaACfQIaBnk7oN6P2Y/iqZnzpmunn0H+ezIdNICosSwqqv0ibjtwrDuOHIFecR0 ALuMkocr+sEetAZRlDVhtLu4DM3el0LGHSf3z/4Tg1A18GF8obxaNrHRMP5qjlILod/+ ibveaM3sPsHwdobzycdq0fFLJ+CIuoZh3bn3cKzD2ylG4pC7q1thJqbAyc6/8KvD2d1x vnWv1RucgU4b8qM7PEsEtted7/d3iK5vnl/dnZXxj0FG3/6k5L8sVsKyXEOF643i1BCy ZaOshG1+fJybJfnWpSniPYhQ2XINX6LTaqw65NLe+a04iSfRPU5L+yNzbfdCZ9zmPSmc ymwA== X-Gm-Message-State: AO0yUKVYdnswZ2lLRZQhpyc834XU3zcQedMEbm8osjaGq9eQnGDy852a H4jnS47EoOA3DfKosXgihAdC/4XiHD7PxT99uWk= X-Google-Smtp-Source: AK7set84x5XkZINYlYKt/7lE019uagqCA6gNBY6L1x+TFhb3AowMVbNGGRUUTldWHfMkZM0vzBuAcCk1FDbgHNugY+I= X-Received: by 2002:a2e:aa98:0:b0:293:51cd:20fa with SMTP id bj24-20020a2eaa98000000b0029351cd20famr2829091ljb.6.1678090321919; Mon, 06 Mar 2023 00:12:01 -0800 (PST) MIME-Version: 1.0 References: <20230302022921.4055291-1-xionghuluo@tencent.com> <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> In-Reply-To: <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> From: Richard Biener Date: Mon, 6 Mar 2023 09:11:49 +0100 Message-ID: Subject: Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] To: Xionghu Luo Cc: gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, rguenther@suse.de, hubicka@ucw.cz Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Mar 6, 2023 at 8:22=E2=80=AFAM 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 =3D __gcov0.f[0]; > > PROF_edge_counter_18 =3D PROF_edge_counter_17 + 1; > > __gcov0.f[0] =3D PROF_edge_counter_18; > > [t.c:3:7] p_6 =3D 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 =3D PHI > > # p_3 =3D 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 =3D p_3 + 1; > > [t.c:8:28] n_13 =3D n_1 + -1; > > [t.c:8:28] if (n_13 !=3D 0) > > goto ; [INV] > > else > > goto ; [INV] > > > > : > > PROF_edge_counter_21 =3D __gcov0.f[2]; > > PROF_edge_counter_22 =3D PROF_edge_counter_21 + 1; > > __gcov0.f[2] =3D PROF_edge_counter_22; > > [t.c:7:5] goto ; [100.00%] > > > > : > > PROF_edge_counter_23 =3D __gcov0.f[3]; > > PROF_edge_counter_24 =3D PROF_edge_counter_23 + 1; > > __gcov0.f[3] =3D PROF_edge_counter_24; > > [t.c:9:16] _14 =3D 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=3D90574 > > .gimple: > > int f (int s, int n) > [small.c:2:1] { > int D.2755; > int p; > > [small.c:3:7] p =3D 0; > [small.c:5:3] switch (s) , [small.c:7:5] case 0: , [small.c:11:5] case 1: > > [small.c:7:5] : <=3D case label > : <=3D loop label > [small.c:8:13] p =3D p + 1; > [small.c:8:26] n =3D n + -1; > [small.c:8:26] if (n !=3D 0) goto ; else goto ; > : > [small.c:9:14] D.2755 =3D p; > [small.c:9:14] return D.2755; > [small.c:11:5] : > : > [small.c:12:13] p =3D p + 1; > [small.c:12:26] n =3D n + -1; > [small.c:12:26] if (n !=3D 0) goto ; else goto ; > : > [small.c:13:14] D.2755 =3D p; > [small.c:13:14] return D.2755; > : > [small.c:16:10] D.2755 =3D 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 =3D 0; > [small.c:5:3] switch (s) [INV], [small.c:7:5] case 0: <= L0> [INV], [small.c:11:5] case 1: [INV]> > > : > [small.c:7:5] : <=3D case 0 > [small.c:8:13 discrim 1] p =3D p + 1; > [small.c:8:26 discrim 1] n =3D n + -1; > [small.c:8:26 discrim 1] if (n !=3D 0) > goto ; [INV] > else > goto ; [INV] > > : > [small.c:9:14] D.2755 =3D p; > [small.c:9:14] goto ; [INV] > > : > [small.c:11:5] : <=3D case 1 > [small.c:12:13 discrim 1] p =3D p + 1; > [small.c:12:26 discrim 1] n =3D n + -1; > [small.c:12:26 discrim 1] if (n !=3D 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 bloc= k: > > > 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 =3D gimple_location (plabel); > + location_t locus =3D gimple_location (label_stmt); > + expanded_location locus_e =3D expand_location (locus); > + > + if (flag_test_coverage && !same_line_p (locus, &locus_e, loc_pr= ev)) > + 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 =3D 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 =3D gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) { gimple *stmt =3D 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) !=3D UNKNOWN_LOCATION) && gimple_location (stmt) !=3D 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 shou= ld be able to put a breakpoint on the label hitting once rather than once each loop iteration. Thanks, Richard. > .profile: > > : > PROF_edge_counter_17 =3D __gcov0.f[0]; > PROF_edge_counter_18 =3D PROF_edge_counter_17 + 1; > __gcov0.f[0] =3D PROF_edge_counter_18; > [small.c:3:7] p_6 =3D 0; > [small.c:5:3] switch (s_7(D)) [INV], [small.c:7:5] case= 0: [INV], [small.c:11:5] case 1: [INV]> > > : > [small.c:7:5] : <=3D case 0 > PROF_edge_counter_19 =3D __gcov0.f[1]; > PROF_edge_counter_20 =3D PROF_edge_counter_19 + 1; > __gcov0.f[1] =3D PROF_edge_counter_20; > > : > # n_1 =3D PHI > # p_3 =3D 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 =3D p_3 + 1; > [small.c:8:26 discrim 1] n_13 =3D n_1 + -1; > [small.c:8:26 discrim 1] if (n_13 !=3D 0) > goto ; [INV] > else > goto ; [INV] > > : > PROF_edge_counter_23 =3D __gcov0.f[3]; > PROF_edge_counter_24 =3D PROF_edge_counter_23 + 1; > __gcov0.f[3] =3D PROF_edge_counter_24; > goto ; [100.00%] > > : > [small.c:9:14] _14 =3D p_12; > [small.c:9:14] goto ; [INV] > > : > [small.c:11:5] : <=3D case 1 > PROF_edge_counter_21 =3D __gcov0.f[2]; > PROF_edge_counter_22 =3D PROF_edge_counter_21 + 1; > __gcov0.f[2] =3D PROF_edge_counter_22; > > > : > # n_2 =3D PHI > # p_4 =3D 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 =3D p_4 + 1; > [small.c:12:26 discrim 1] n_10 =3D n_2 + -1; > [small.c:12:26 discrim 1] if (n_10 !=3D 0) > goto ; [INV] > else > goto ; [INV] > > : > PROF_edge_counter_25 =3D __gcov0.f[4]; > PROF_edge_counter_26 =3D PROF_edge_counter_25 + 1; > __gcov0.f[4] =3D PROF_edge_counter_26; > goto ; [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 =3D 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);}