From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id 9F5B43858D33; Thu, 2 Mar 2023 10:22:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9F5B43858D33 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-pl1-x62b.google.com with SMTP id i3so17047066plg.6; Thu, 02 Mar 2023 02:22:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1677752577; 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=3dirIVfuWmpytHoty1dc8C4r0CaJ7H7cXt7aD6YGN3k=; b=XdPNiIHDMYn/Aa7HQZDwqyuypL1eJZjukvZ8+dtN9ZDHDtKJqomM4W+HleIJpM4+XR AigeQS9HN/J34zfk2Sartd6jZBTyHN4xHR4Nw8VKNlIFkyaST3XwuGaiz2tiSFcMEtW3 ns2mThvE9eKcw84shq27biHZMtDRjMJ15RtGSfx8noqbOAyG6BhU1snEDKhAZZIwjGol /q0OGUehvpjEOD13RFVkKDKbWwcl3k3lB1lYkv8tsQ28XmpEKg0qja09hvopVGhOspnt 9Q92i7vTrMwVia3rTCGwCrwexGQr1kJ9WdhP3o61uV0NbARjcRKprIL2IWD+2aPaZOhC c/qQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677752577; 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=3dirIVfuWmpytHoty1dc8C4r0CaJ7H7cXt7aD6YGN3k=; b=SD440dZgSdPfMzYueQWIUN8tmUQoTvoNPTMRgFfmxUjZS0oEJtzTcy0vXeMlxAb9cn sY/qPxrimTaUfvE7drGYfgA3zt9qdMasWzBUKoUCvdIyfjPLTpxnD86oFkIDo2StpYHp xLcJMwa18pqMrHsSu4IdD2tmu6olMreRkPBPklrk+AlwgPCwx0vgBTtiTJ+yVAPfc5ov t+x+JTdU4e36W1FFPFWcuP8OyrFnnRE5WNEh5a0WwwtUnxudQHrog3UjtwMDV+aS8crq P/ol9ZN4elPzVZqtryUc1sUTOnrslxvK3wAyoWIG1WTQcKmlW/7u8nHK+8F4TpdaaFYo NFsw== X-Gm-Message-State: AO0yUKWVh/OL1tYIGsf6Pn835iPIsGW89kaaPHR5kZggzqQWyMFYe+mm M4cEzkOYcnhe4wrD7Y2zfOE= X-Google-Smtp-Source: AK7set+kQVmJLMoeXI58khpblnYj1Wl7ehggPzvmqJ7QJudbPpcE5ghwJFF3B3gx1bFxmE7awY9zTQ== X-Received: by 2002:a17:903:48a:b0:19d:90f:6c49 with SMTP id jj10-20020a170903048a00b0019d090f6c49mr8975966plb.68.1677752577516; Thu, 02 Mar 2023 02:22:57 -0800 (PST) Received: from [192.168.255.10] ([103.7.29.32]) by smtp.gmail.com with ESMTPSA id p19-20020a1709028a9300b0019cec7d88c3sm10064567plo.236.2023.03.02.02.22.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Mar 2023 02:22:57 -0800 (PST) Message-ID: <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> Date: Thu, 2 Mar 2023 18:22:40 +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 , Xionghu Luo Cc: gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, rguenther@suse.de, hubicka@ucw.cz References: <20230302022921.4055291-1-xionghuluo@tencent.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=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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 16:41, Richard Biener wrote: > On Thu, Mar 2, 2023 at 3:31 AM Xionghu Luo via Gcc-patches > wrote: >> >> When spliting edge with self loop, the split edge should be placed just next to >> the edge_in->src, otherwise it may generate different position latch bbs for >> two consecutive self loops. For details, please refer to: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93680#c4 >> >> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for >> master? >> >> gcc/ChangeLog: >> >> PR gcov/93680 >> * tree-cfg.cc (split_edge_bb_loc): Return edge_in->src for self loop. >> >> gcc/testsuite/ChangeLog: >> >> PR gcov/93680 >> * gcc.misc-tests/gcov-pr93680.c: New test. >> >> Signed-off-by: Xionghu Luo >> --- >> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 +++++++++++++++++++++ >> gcc/tree-cfg.cc | 2 +- >> 2 files changed, 25 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c >> >> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c >> new file mode 100644 >> index 00000000000..b2bf9e626fc >> --- /dev/null >> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c >> @@ -0,0 +1,24 @@ >> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */ >> +/* { dg-do run { target native } } */ >> + >> +int f(int s, int n) >> +{ >> + int p = 0; >> + >> + switch (s) >> + { >> + case 0: /* count(5) */ >> + do { p++; } while (--n); /* count(5) */ >> + return p; /* count(1) */ >> + >> + case 1: /* count(5) */ >> + do { p++; } while (--n); /* count(5) */ >> + return p; /* count(1) */ >> + } >> + >> + return 0; >> +} >> + >> +int main() { f(0, 5); f(1, 5); return 0; } >> + >> +/* { dg-final { run-gcov gcov-pr93680.c } } */ >> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc >> index a9fcc7fd050..6fa1d83d366 100644 >> --- a/gcc/tree-cfg.cc >> +++ b/gcc/tree-cfg.cc >> @@ -3009,7 +3009,7 @@ split_edge_bb_loc (edge edge_in) >> if (dest_prev) >> { >> edge e = find_edge (dest_prev, dest); >> - if (e && !(e->flags & EDGE_COMPLEX)) >> + if ((e && !(e->flags & EDGE_COMPLEX)) || edge_in->src == edge_in->dest) > > I think this should eventually apply to all backedge edge_in, correct? > But of course > we cannot easily test for this here. > > Still since this affects ordering in the {next,prev}_bb chain only but not CFG > semantics I wonder how it can affect coverage? Isn't it only by chance that > this block order survives? For case: 1 int f(int s, int n) 2 { 3 int p = 0; 4 int q = 0; 5 6 switch (s) 7 { 8 case 0: 9 do { p++; } while (--n); 10 return p; 11 12 case 1: 13 do { p++; } while (--n); 14 return p; 15 } 16 17 return 0; 18 } 19 20 int main() { f(0, 5); f(1, 5);} current GCC generates: : ... : <= first loop ... goto ; [INV] else goto ; [INV] : <= first latch bb goto ; [100.00%] : ... goto ; [INV] : <= second latch bb : <= second loop ... goto ; [INV] else goto ; [INV] and are created by split_edge->split_edge_bb_loc, is located after the loop, but is located before the loop. First call of split_edge_bb_loc, the dest_prev is , and find_edge did find a edge from to , the returned afte_bb is , so latch is put after the loop but second call of split_edge_bb_loc, the dest_prev is , so find_edge return 0, and the returned after_bb is , then the created latch is put before the loop... Different latch bb position caused different gcno, while gcov has poor information and not that smart to recognize it:(, is it reasonable to keep this kind of loops same order? 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 > > For the case when both edge_in->src has more than one successor and > edge_in->dest has more than one predecessor there isn't any good heuristic > to make printing the blocks in chain order "nice" (well, the backedge > one maybe). > > But as said - this order shouldn't have any effect on semantics ... > >> return edge_in->src; >> } >> return dest_prev; >> -- >> 2.27.0 >>