From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by sourceware.org (Postfix) with ESMTPS id 2CC883858D3C; Tue, 7 Mar 2023 10:26:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2CC883858D3C 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-x102a.google.com with SMTP id 6-20020a17090a190600b00237c5b6ecd7so16038746pjg.4; Tue, 07 Mar 2023 02:26:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678184805; 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=MXt9zke2Dv2tczk6dH9kYE3OD6KD9eoKuJaJFd/MtBI=; b=UuV/A1RJnIFTJhsOl7mkuH2X/TtRc00Mu1tZCdBI9oCNiHHwBJraB9HyB24MZ9hs+v n6UsxmKKo/1wJn4LBfduwLfaYSWpbgnJVMrtc+pwAnJs+06H3Ioxg7/MtSs+JmyljHnE rkI7f7Saww0zPv56MqcmMZUoETORRyNITa+LyTo2xoIkZkoH8bJ0J2Ti/1B5OA241D5s BFOPq8HZoJaA3gn2NvwAXpjGQsNfmWFdbahLFfEEdLkjCUT+G0WatVFV0VXkcz30Bo8O sNI7CmfD5y6lvqTe3t9fbBc7D8jSNKdJ0dqADPyzufXsLOhYYkACwidvoZyvdOnbMb36 8EFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678184805; 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=MXt9zke2Dv2tczk6dH9kYE3OD6KD9eoKuJaJFd/MtBI=; b=beOI49lczcCDvePB8z+FCU36Y++/AX9XtjO98uEKyb3rXz1OQgW1F3laj4QjulIbvh PU66iGTXO2SNA6zqK79GyPheylQ2jkNe0o6grJB3grt9gvN+amocfnn9bop0T0cARklg WF4c7fgkvZRP5hNMTYl86UHyu3WIMUUG5cTRe7skAP/kpdFeuzkXgJWFG5ZGys5Sactj E8RwFJIoWRO6ocaumFAFiSltzytVvT6zDUtuWN/hUP5bDSRkCgpcoHonmiKUdoEeTOis XQmKkTZtQPmLV2ao/kG/h7ndJJaxlLnWsf9TQcWInn8/QcGtHCQKZBFjUg72B3JrPCvH a8Nw== X-Gm-Message-State: AO0yUKUDCAqh33bSLmmt9B+VgBR6a/5CaH7RCsmMq2YOCt0U3laDXPBz 610TfQ66chQ8cWSczpoeh9g= X-Google-Smtp-Source: AK7set8fDpqHml1OFxlUksXxtkS/ClbMBkKhGj+lg1+gCYeBlFPhZKcSRANs9Zy81AgtwHEULfGi6A== X-Received: by 2002:a17:903:441:b0:19b:fa9:678b with SMTP id iw1-20020a170903044100b0019b0fa9678bmr14037048plb.40.1678184805056; Tue, 07 Mar 2023 02:26:45 -0800 (PST) Received: from [192.168.255.10] ([103.7.29.32]) by smtp.gmail.com with ESMTPSA id lg14-20020a170902fb8e00b0019a7f493151sm8144835plb.212.2023.03.07.02.26.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Mar 2023 02:26:44 -0800 (PST) Message-ID: Date: Tue, 7 Mar 2023 18:26:38 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH 1/2] gcov: Fix "do-while" structure in case statement leads to incorrect code coverage [PR93680] To: Richard Biener Cc: Richard Biener , gcc-patches@gcc.gnu.org, luoxhu@gcc.gnu.org, hubicka@ucw.cz References: <20230302022921.4055291-1-xionghuluo@tencent.com> <00510a56-1fe5-a556-21cf-fa869f70800b@gmail.com> <8011e540-119f-f155-a32c-a3e739a64ac7@gmail.com> From: Xionghu Luo In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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/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? Thanks, Xionghu