From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 85FFE3858D39; Tue, 7 Mar 2023 11:25:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 85FFE3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 55059218FD; Tue, 7 Mar 2023 11:25:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1678188342; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=gzYA38Ao2xnYlxc8IjCWTYw95ClcfvT5XOZsLG5UwQo=; b=RCdnxqPow+PtMKN2YbGAYODZh+uPoBA2qfisRjPISJUl1lrP4crpE6K4V3sX4w6pmIDBA3 RJOvDhz+9jqiy+YrcOmoQjNFPjxM0mnQdUh1m33nfEB9Rd7VKggYZUf10/CKY1vYtLr2yX 22DspdsAmh9aSl7GIGBfF9PF0WGr9ec= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1678188342; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=gzYA38Ao2xnYlxc8IjCWTYw95ClcfvT5XOZsLG5UwQo=; b=nkILXb2uqKBj7bovHd3YuXZYaHsJPAxMsLiTzdxeAIxc/24GE2Nnwqv+wPaM4JZySwTtKE VegTpnGDI6ALgpAw== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 357102C141; Tue, 7 Mar 2023 11:25:42 +0000 (UTC) Date: Tue, 7 Mar 2023 11:25:42 +0000 (UTC) From: Richard Biener To: Xionghu Luo cc: Richard Biener , gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, hubicka@ucw.cz Subject: Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] In-Reply-To: Message-ID: References: <20230302022921.4055291-1-xionghuluo@tencent.com> <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="-1609957120-1212123862-1678188342=:18795" X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609957120-1212123862-1678188342=:18795 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Tue, 7 Mar 2023, Xionghu Luo wrote: > > > On 2023/3/7 16:53, Richard Biener wrote: > > On Tue, 7 Mar 2023, Xionghu Luo wrote: > > >> 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. > > > > OK, the issue is that make_edges_bb does nothing for an empty block > > but it should at least create a fallthru edge here. Thus, > > > > if (!last) > > fallthru = true; > > > > else > > switch (gimple_code (last)) > > { > > > > instead of simply returning if (!last). The alternative would be > > to make sure that cleanup_dead_labels preserves at least one > > statement in a block. > > > > Looking at the testcases I wonder if preserving all the fallthru labels > > is really necessary - for coverage we should have a counter ready. For > > the testcase we arrive with > > > > L.1: > > L.2: > > L.3: > > L.4: > > i = 1; > > It was: > > : > > : > L.1: > > : > L.2: > > : > L.3: > > : > L.4: > > : > L.5: > > : > L.6: > return; > > : > > before the second call of cleanup_dead_labels, after it, all labels are > removed, then tree_forwarder_block_p remove all forworders. Yes, it > creates blocks and remove blocks immediately... > > > > > where the frontend simplified things but put labels at each line. > > I suppose we could optimize this by re-computing TREE_USED and only > > splitting before labels reached by a control statement? That would > > cover the backedge case in the original testcase. cleanup_dead_labels > > does something like that already. > > > >> 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 (); > > > > That probably makes sense. Looking at group_case_labels () that also > > seems to do unwanted things (to debugging and coverage), its comment > > says that for > > > > switch (i) > > { > > case 1: > > /* fallthru */ > > case 2: > > /* fallthru */ > > case 3: > > k = 0; > > > > it would replace that with > > > > case 1..3: > > k = 0; > > > > but that also fails to produce correct coverage, right? Likewise > > setting breakpoints. > > Yes. Should also exclude this. > > > > > Does preserving the labels help setting a goto_locus for the > > fallthru edges? I don't see any code doing that, so > > CFG cleanup will remove the forwarders we created again. > > For the backedge case with switch-case-do-while, tree_forwarder_block_p > returns false when iterating the statement check. > The new created with only one case label instruction still owns > location information in it, so CFG cleanup won't remove the forwarders. > > 390│ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi)) > 391│ { > 392│ gimple *stmt = gsi_stmt (gsi); > 393│ > 394│ switch (gimple_code (stmt)) > 395│ { > 396│ case GIMPLE_LABEL: > 397│ if (DECL_NONLOCAL (gimple_label_label (as_a (stmt)))) > 398│ return false; > 399│ if (!optimize > 400│ && (gimple_has_location (stmt) > 401│ || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION) > 402│ && gimple_location (stmt) != locus) > 403├> return false; > 404│ break; > > > (gdb) ps stmt > : > (gdb) p gimple_location (stmt) > $154 = 2147483656 > (gdb) pel $154 > {file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp > = false} > (gdb) > (gdb) pbb bb > ;; basic block 3, loop depth 0 > ;; pred: 2 > : > ;; succ: 4 > > > > > It would be nice to avoid creating blocks / preserving labels we'll > > immediately remove again. For that we do need some analysis > > before creating basic-blocks that determines whether a label is > > possibly reached by a non-falltru edge. > > > > > : > p = 0; > switch (s) , case 0: , case 1: > > > : > : <= prev_stmt > : <= stmt > p = p + 1; > n = n + -1; > if (n != 0) goto ; else goto ; > > Check if is a case label and is a goto target then return true > in stmt_starts_bb_p to start a new basic block? This would avoid creating and > removing blocks, but cleanup_dead_labels has all bbs setup while > stmt_starts_bb_p > does't yet to iterate bbs/labels to establish label_for_bb[] map? Yes. I think we'd need something more pragmatic before make_blocks (), like re-computing TREE_USED of the label decls or computing a bitmap of targeted labels (targeted by goto, switch or any other means). I'll note that doing a cleanup_dead_labels () like optimization before we create blocks will help keeping LABEL_DECL_UID and thus label_to_block_map dense. But it does look like a bit of an chicken-and-egg problem and the question is how effective the dead label removal is in practice. Richard. ---1609957120-1212123862-1678188342=:18795--