From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) by sourceware.org (Postfix) with ESMTPS id 8C0263858D35; Mon, 6 Mar 2023 07:22:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8C0263858D35 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-pj1-x102e.google.com with SMTP id bo22so8833746pjb.4; Sun, 05 Mar 2023 23:22:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678087340; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=1bIbN9aRgGJSnZ+z9hHgVAxr0jTvWFw4QiqUdE42LIM=; b=CJauF9cuSIqWuj6JegPx/4tNt/+LYCxcS+u6fq0+lQLT8D6fpGjwI2DwRHw+iZjvGO j+a5coaBCATx8xsnL6/io2m39vUaWvF1NcW4kdNJhHJwknqvJvUndes0AG2TYF0NohZj wNcwFRgpwqjSd03zoJIi9RVJ3Kxb4wchgk52ExFHYHyIF3LW2C2PiivmSubwV7vxR2tR 0/hnQHp6eHyP7+l9O6aSgJWKwI40j38QQ/7ufna9W+wsmjA3qbvU7KgKh4nD3IS99317 BuVyQELuNpOnixMRdS7sFCad0UyW1DGwszkeZHTCNnOvwiIsHQjCKl7UOnTFIsRu1F+w KpDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678087340; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=1bIbN9aRgGJSnZ+z9hHgVAxr0jTvWFw4QiqUdE42LIM=; b=crmB164KbbiqDnUEf6fmPnofUr+scyuMwyppnjZlBKkrp6gPkR/+rdt04P4tFlFaxc zQua6O9A5MgUd0RQiQh3n/JpxnwJrIT+HduG6Q2skwtCfz+jXhn1oFAfBCY8ADhayflN cZrxgDH/6kleXx3kH5cVCh71m8SzJ3RJyvGeM824eCpa7lDuJUBx2GduAlAbBeFa3CkP AzUCfp/qWDeB9znjUoMPuP+sWDTuLlj+U1Ce8lYrwfdbFAMQYlxrL4paK2IzC9e4r2gX LjxPM+IXNQ3N3ZeCBQlLOmZgG3MCU3iViW+2QTrmMTJs2oOyjbNjPJyohMLewN8JNVCu 17bw== X-Gm-Message-State: AO0yUKXqJRmLJJDe1lTbnIRHWjT8X0qJ8MybN6ZykFwQDkqwD/CRx0va YdEq9L9O740HRs63FLLp5mM= X-Google-Smtp-Source: AK7set+1Tne+bOlrD3BTZWxypkpXtn2m4mzBkJw/Ta0hTYKzA5SkzgBCgK5E05KDa3Z/sYXPx9gs/Q== X-Received: by 2002:a17:902:ec8f:b0:19a:aaac:f4d1 with SMTP id x15-20020a170902ec8f00b0019aaaacf4d1mr9084784plg.13.1678087340442; Sun, 05 Mar 2023 23:22:20 -0800 (PST) Received: from [192.168.255.10] ([103.7.29.32]) by smtp.gmail.com with ESMTPSA id ji3-20020a170903324300b0019a9637b2d3sm5914016plb.279.2023.03.05.23.22.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 05 Mar 2023 23:22:20 -0800 (PST) Message-ID: <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> Date: Mon, 6 Mar 2023 15:22:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Subject: Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] To: Richard Biener Cc: gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, rguenther@suse.de, hubicka@ucw.cz References: <20230302022921.4055291-1-xionghuluo@tencent.com> <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> From: Xionghu Luo In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,NICE_REPLY_A,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 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; } .profile: : 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)) [INV], [small.c:7:5] case 0: [INV], [small.c:11:5] case 1: [INV]> : [small.c:7:5] : <= 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; : # n_1 = PHI # 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 ; [INV] else goto ; [INV] : PROF_edge_counter_23 = __gcov0.f[3]; PROF_edge_counter_24 = PROF_edge_counter_23 + 1; __gcov0.f[3] = PROF_edge_counter_24; goto ; [100.00%] : [small.c:9:14] _14 = p_12; [small.c:9:14] goto ; [INV] : [small.c:11:5] : <= 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; : # n_2 = PHI # 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 ; [INV] else goto ; [INV] : PROF_edge_counter_25 = __gcov0.f[4]; PROF_edge_counter_26 = PROF_edge_counter_25 + 1; __gcov0.f[4] = 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 = 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);}