Message ID | e051fabe-b66d-5bb2-d317-f82a34f4088b@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 40054385840B for <patchwork@sourceware.org>; Wed, 2 Mar 2022 23:15:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 40054385840B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1646262941; bh=o2hOgiO78CWHJVJIufzLMrpn11sjCyp3vha02LF22TY=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=CAZSiiFfJweBghLtrRcoIk70Msn68DYDJz4tqSAoqyQwOkadZ+aU/mlbvMMLjHAKu /kNvNc5eorCSetjSGvV/p72ii3LF/G2So4CYVvNRFmjTxPLwgIqxi6dKnoisdFKL5f qxDoTWoXWbd3VNZsEib0bLklxhc8XWGVY0a56uc8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-il1-x132.google.com (mail-il1-x132.google.com [IPv6:2607:f8b0:4864:20::132]) by sourceware.org (Postfix) with ESMTPS id A6BB13858D39 for <gcc-patches@gcc.gnu.org>; Wed, 2 Mar 2022 23:15:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A6BB13858D39 Received: by mail-il1-x132.google.com with SMTP id w4so2672678ilj.5 for <gcc-patches@gcc.gnu.org>; Wed, 02 Mar 2022 15:15:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent :content-language:to:cc:from:subject; bh=o2hOgiO78CWHJVJIufzLMrpn11sjCyp3vha02LF22TY=; b=obfI9zysXWRimCbi053f1CkhbTuP8xayHMjG9CcELmvxA0Y1zoTd7+VXCOT41FFlLl q8D554ioIyGoBsYeJ7Bi9NLDfOnpJS+2Cw+IVCwir/xDvdUG723XpVNsWrp5k7Sx58Nb yfgWhoZRI7df0hCKX2d9CjOtbzH20uDj8p5m1UnnTgJDucVipSeH3EySwFWCM8MNnmVa mWxSBvPP9oNlZLC1EJjr5ZYVw8zlsljblDFKY0zqqsycQcaFluI1suZam8F9eD8rr8aF 3+M7ZTQnwS5na0curYRkKIuTFSmUOMW300/5DTy27W07ZzDcrGKFqmbt/eu44Hv6l9hu 6zTQ== X-Gm-Message-State: AOAM532E0DHxlbxv0PBtTcB/57FnWW5RQVlAr+n5DUwoMDXgrEoBQtWC zGCMKCSkDDYMpoxKb8R/IHx7NfyQOx4= X-Google-Smtp-Source: ABdhPJxKLj62t/qk8vQrv+3AmM0UZqZL+sk2qenJR/Gf73Gj+61Q4GcQc69RMa84IMAkoXSCvqzFBA== X-Received: by 2002:a05:6e02:1bc5:b0:2c2:7bc9:8e8f with SMTP id x5-20020a056e021bc500b002c27bc98e8fmr29344647ilv.5.1646262910856; Wed, 02 Mar 2022 15:15:10 -0800 (PST) Received: from [192.168.0.41] (97-118-101-70.hlrn.qwest.net. [97.118.101.70]) by smtp.gmail.com with ESMTPSA id k10-20020a6b7e4a000000b00640a8142cbdsm227520ioq.49.2022.03.02.15.15.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Mar 2022 15:15:10 -0800 (PST) Content-Type: multipart/mixed; boundary="------------hv1gf9082xlPICMtWB4il6ZG" Message-ID: <e051fabe-b66d-5bb2-d317-f82a34f4088b@gmail.com> Date: Wed, 2 Mar 2022 16:15:09 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Content-Language: en-US To: gcc-patches <gcc-patches@gcc.gnu.org> Subject: [PATCH] call mark_dfs_back_edges() before testing EDGE_DFS_BACK [PR104761] X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Martin Sebor <msebor@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
call mark_dfs_back_edges() before testing EDGE_DFS_BACK [PR104761]
|
|
Commit Message
Martin Sebor
March 2, 2022, 11:15 p.m. UTC
The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never calls the mark_dfs_back_edges() function that initializes the bit (I didn't know about it). As a result the bit is not set when expected, which can cause false positives under the right conditions. The attached patch adds a call to the warning pass to initialize the bit. Tested on x86_64-linux. Martin
Comments
On Wed, Mar 02, 2022 at 04:15:09PM -0700, Martin Sebor via Gcc-patches wrote: > The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never > calls the mark_dfs_back_edges() function that initializes the bit (I > didn't know about it). As a result the bit is not set when expected, > which can cause false positives under the right conditions. Not a review because I also had to look up what computes EDGE_DFS_BACK, so I don't feel the right person to ack the patch. So, just a few questions. The code in question is: auto gsi = gsi_for_stmt (use_stmt); auto_bitmap visited; /* A use statement in the last basic block in a function or one that falls through to it is after any other prior clobber of the used variable unless it's followed by a clobber of the same variable. */ basic_block bb = use_bb; while (bb != inval_bb && single_succ_p (bb) && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK))) { if (!bitmap_set_bit (visited, bb->index)) /* Avoid cycles. */ return true; for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) { gimple *stmt = gsi_stmt (gsi); if (gimple_clobber_p (stmt)) { if (clobvar == gimple_assign_lhs (stmt)) /* The use is followed by a clobber. */ return false; } } bb = single_succ (bb); gsi = gsi_start_bb (bb); } 1) shouldn't it give up for EDGE_ABNORMAL too? I mean, e.g. following a non-local goto forced edge from a noreturn call to a non-local label (if there is just one) doesn't seem right to me 2) if EDGE_DFS_BACK is computed and 1) is done, is there any reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK check as well as the visited bitmap (and having them use very different answers, if EDGE_DFS_BACK is seen, the function will return false, if visited bitmap has a bb, it will return true)? Can't the visited bitmap go away? 3) I'm concerned about compile time with the above, consider you have 1000000 use_stmts and 1000000 corresponding inv_stmts and in each case you enter this loop and go through a series of very large basic blocks that don't clobber those stmts; shouldn't it bail out (return false) after walking some param controlled number of non-debug stmts (say 1000 by default)? There is an early exit if if (dominated_by_p (CDI_DOMINATORS, use_bb, inval_bb)) return true; (I admit I haven't read the code what happens if there is more than one clobber for the same variable) > The attached patch adds a call to the warning pass to initialize > the bit. Tested on x86_64-linux. > > Martin > Call mark_dfs_back_edges before testing EDGE_DFS_BACK [PR104761]. > > Resolves: > PR middle-end/104761 - bogus -Wdangling-pointer with cleanup and infinite loop > > gcc/ChangeLog: > > PR middle-end/104761 > * gimple-ssa-warn-access.cc (pass_waccess::execute): Call > mark_dfs_back_edges. > > gcc/testsuite/ChangeLog: > > PR middle-end/104761 > * g++.dg/warn/Wdangling-pointer-4.C: New test. > * gcc.dg/Wdangling-pointer-4.c: New test. > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index b7cdad517b3..b519712d76e 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -47,7 +47,7 @@ > #include "tree-object-size.h" > #include "tree-ssa-strlen.h" > #include "calls.h" > -#include "cfgloop.h" > +#include "cfganal.h" > #include "intl.h" > #include "gimple-range.h" > #include "stringpool.h" > @@ -4710,6 +4710,9 @@ pass_waccess::execute (function *fun) > calculate_dominance_info (CDI_DOMINATORS); > calculate_dominance_info (CDI_POST_DOMINATORS); > > + /* Set or clear EDGE_DFS_BACK bits on back edges. */ > + mark_dfs_back_edges (fun); > + > /* Create a new ranger instance and associate it with FUN. */ > m_ptr_qry.rvals = enable_ranger (fun); > m_func = fun; Jakub
> Am 03.03.2022 um 09:02 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>: > > On Wed, Mar 02, 2022 at 04:15:09PM -0700, Martin Sebor via Gcc-patches wrote: >> The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never >> calls the mark_dfs_back_edges() function that initializes the bit (I >> didn't know about it). As a result the bit is not set when expected, >> which can cause false positives under the right conditions. > > Not a review because I also had to look up what computes EDGE_DFS_BACK, > so I don't feel the right person to ack the patch. The patch looks OK. The questions below might be all valid but they can be addressed with followup changes. Richard. > So, just a few questions. > > The code in question is: > auto gsi = gsi_for_stmt (use_stmt); > > auto_bitmap visited; > > /* A use statement in the last basic block in a function or one that > falls through to it is after any other prior clobber of the used > variable unless it's followed by a clobber of the same variable. */ > basic_block bb = use_bb; > while (bb != inval_bb > && single_succ_p (bb) > && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK))) > { > if (!bitmap_set_bit (visited, bb->index)) > /* Avoid cycles. */ > return true; > > for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > { > gimple *stmt = gsi_stmt (gsi); > if (gimple_clobber_p (stmt)) > { > if (clobvar == gimple_assign_lhs (stmt)) > /* The use is followed by a clobber. */ > return false; > } > } > > bb = single_succ (bb); > gsi = gsi_start_bb (bb); > } > > 1) shouldn't it give up for EDGE_ABNORMAL too? I mean, e.g. > following a non-local goto forced edge from a noreturn call > to a non-local label (if there is just one) doesn't seem > right to me > 2) if EDGE_DFS_BACK is computed and 1) is done, is there any > reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK > check as well as the visited bitmap (and having them use > very different answers, if EDGE_DFS_BACK is seen, the function > will return false, if visited bitmap has a bb, it will return true)? > Can't the visited bitmap go away? > 3) I'm concerned about compile time with the above, consider you have > 1000000 use_stmts and 1000000 corresponding inv_stmts and in each > case you enter this loop and go through a series of very large basic > blocks that don't clobber those stmts; shouldn't it bail out > (return false) after walking some param > controlled number of non-debug stmts (say 1000 by default)? > There is an early exit if > if (dominated_by_p (CDI_DOMINATORS, use_bb, inval_bb)) > return true; > (I admit I haven't read the code what happens if there is more than > one clobber for the same variable) > >> The attached patch adds a call to the warning pass to initialize >> the bit. Tested on x86_64-linux. >> >> Martin > >> Call mark_dfs_back_edges before testing EDGE_DFS_BACK [PR104761]. >> >> Resolves: >> PR middle-end/104761 - bogus -Wdangling-pointer with cleanup and infinite loop >> >> gcc/ChangeLog: >> >> PR middle-end/104761 >> * gimple-ssa-warn-access.cc (pass_waccess::execute): Call >> mark_dfs_back_edges. >> >> gcc/testsuite/ChangeLog: >> >> PR middle-end/104761 >> * g++.dg/warn/Wdangling-pointer-4.C: New test. >> * gcc.dg/Wdangling-pointer-4.c: New test. >> >> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc >> index b7cdad517b3..b519712d76e 100644 >> --- a/gcc/gimple-ssa-warn-access.cc >> +++ b/gcc/gimple-ssa-warn-access.cc >> @@ -47,7 +47,7 @@ >> #include "tree-object-size.h" >> #include "tree-ssa-strlen.h" >> #include "calls.h" >> -#include "cfgloop.h" >> +#include "cfganal.h" >> #include "intl.h" >> #include "gimple-range.h" >> #include "stringpool.h" >> @@ -4710,6 +4710,9 @@ pass_waccess::execute (function *fun) >> calculate_dominance_info (CDI_DOMINATORS); >> calculate_dominance_info (CDI_POST_DOMINATORS); >> >> + /* Set or clear EDGE_DFS_BACK bits on back edges. */ >> + mark_dfs_back_edges (fun); >> + >> /* Create a new ranger instance and associate it with FUN. */ >> m_ptr_qry.rvals = enable_ranger (fun); >> m_func = fun; > > Jakub >
On 3/3/2022 1:01 AM, Jakub Jelinek wrote: > On Wed, Mar 02, 2022 at 04:15:09PM -0700, Martin Sebor via Gcc-patches wrote: >> The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never >> calls the mark_dfs_back_edges() function that initializes the bit (I >> didn't know about it). As a result the bit is not set when expected, >> which can cause false positives under the right conditions. > Not a review because I also had to look up what computes EDGE_DFS_BACK, > so I don't feel the right person to ack the patch. > > So, just a few questions. > > The code in question is: > auto gsi = gsi_for_stmt (use_stmt); > > auto_bitmap visited; > > /* A use statement in the last basic block in a function or one that > falls through to it is after any other prior clobber of the used > variable unless it's followed by a clobber of the same variable. */ > basic_block bb = use_bb; > while (bb != inval_bb > && single_succ_p (bb) > && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK))) > { > if (!bitmap_set_bit (visited, bb->index)) > /* Avoid cycles. */ > return true; > > for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > { > gimple *stmt = gsi_stmt (gsi); > if (gimple_clobber_p (stmt)) > { > if (clobvar == gimple_assign_lhs (stmt)) > /* The use is followed by a clobber. */ > return false; > } > } > > bb = single_succ (bb); > gsi = gsi_start_bb (bb); > } > > 1) shouldn't it give up for EDGE_ABNORMAL too? I mean, e.g. > following a non-local goto forced edge from a noreturn call > to a non-local label (if there is just one) doesn't seem > right to me I think so. > 2) if EDGE_DFS_BACK is computed and 1) is done, is there any > reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK > check as well as the visited bitmap (and having them use > very different answers, if EDGE_DFS_BACK is seen, the function > will return false, if visited bitmap has a bb, it will return true)? > Can't the visited bitmap go away? I would think so. Given how this code is written, I don't see any way other than cycles to visit a BB more than once and with backedges marked, there shouldn't be a way to get into a cycle if we ignore backedges. > 3) I'm concerned about compile time with the above, consider you have > 1000000 use_stmts and 1000000 corresponding inv_stmts and in each > case you enter this loop and go through a series of very large basic > blocks that don't clobber those stmts; shouldn't it bail out > (return false) after walking some param > controlled number of non-debug stmts (say 1000 by default)? > There is an early exit if > if (dominated_by_p (CDI_DOMINATORS, use_bb, inval_bb)) > return true; > (I admit I haven't read the code what happens if there is more than > one clobber for the same variable) I'll let Martin comment on the time complexity question I think #1 and #2 can be addressed as followups. jeff
On 3/3/22 01:01, Jakub Jelinek wrote: > On Wed, Mar 02, 2022 at 04:15:09PM -0700, Martin Sebor via Gcc-patches wrote: >> The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never >> calls the mark_dfs_back_edges() function that initializes the bit (I >> didn't know about it). As a result the bit is not set when expected, >> which can cause false positives under the right conditions. > > Not a review because I also had to look up what computes EDGE_DFS_BACK, > so I don't feel the right person to ack the patch. > > So, just a few questions. > > The code in question is: > auto gsi = gsi_for_stmt (use_stmt); > > auto_bitmap visited; > > /* A use statement in the last basic block in a function or one that > falls through to it is after any other prior clobber of the used > variable unless it's followed by a clobber of the same variable. */ > basic_block bb = use_bb; > while (bb != inval_bb > && single_succ_p (bb) > && !(single_succ_edge (bb)->flags & (EDGE_EH|EDGE_DFS_BACK))) > { > if (!bitmap_set_bit (visited, bb->index)) > /* Avoid cycles. */ > return true; > > for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi)) > { > gimple *stmt = gsi_stmt (gsi); > if (gimple_clobber_p (stmt)) > { > if (clobvar == gimple_assign_lhs (stmt)) > /* The use is followed by a clobber. */ > return false; > } > } > > bb = single_succ (bb); > gsi = gsi_start_bb (bb); > } > > 1) shouldn't it give up for EDGE_ABNORMAL too? I mean, e.g. > following a non-local goto forced edge from a noreturn call > to a non-local label (if there is just one) doesn't seem > right to me Possibly yes. I can add it but I don't have a lot of experience with these bits so if you can suggest a test case to exercise this that would be helpful. > 2) if EDGE_DFS_BACK is computed and 1) is done, is there any > reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK > check as well as the visited bitmap (and having them use > very different answers, if EDGE_DFS_BACK is seen, the function > will return false, if visited bitmap has a bb, it will return true)? > Can't the visited bitmap go away? Possibly. As I said above, I don't have enough experience with these bits to make (and test) the changes quickly, or enough bandwidth to come up to speed on them. Please feel free to make these improvements. > 3) I'm concerned about compile time with the above, consider you have > 1000000 use_stmts and 1000000 corresponding inv_stmts and in each > case you enter this loop and go through a series of very large basic > blocks that don't clobber those stmts; shouldn't it bail out > (return false) after walking some param > controlled number of non-debug stmts (say 1000 by default)? > There is an early exit if > if (dominated_by_p (CDI_DOMINATORS, use_bb, inval_bb)) > return true; > (I admit I haven't read the code what happens if there is more than > one clobber for the same variable) I tend to agree that the loop is less than optimal. But before imposing another arbitrary limit my preference would be to see if it could be made more efficient. Without thinking about it too hard, it seems that with some efficient lookup table a single traversal per function should be sufficient. The first time through populate the table with the clobbered variables along the path from use_bb and each subsequent time just look up clobvar in the table. But I have to use up the rest of my 2021 PTO next week before I lose it and I don't expect to have the cycles to work on this anytime soon. Martin > >> The attached patch adds a call to the warning pass to initialize >> the bit. Tested on x86_64-linux. >> >> Martin > >> Call mark_dfs_back_edges before testing EDGE_DFS_BACK [PR104761]. >> >> Resolves: >> PR middle-end/104761 - bogus -Wdangling-pointer with cleanup and infinite loop >> >> gcc/ChangeLog: >> >> PR middle-end/104761 >> * gimple-ssa-warn-access.cc (pass_waccess::execute): Call >> mark_dfs_back_edges. >> >> gcc/testsuite/ChangeLog: >> >> PR middle-end/104761 >> * g++.dg/warn/Wdangling-pointer-4.C: New test. >> * gcc.dg/Wdangling-pointer-4.c: New test. >> >> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc >> index b7cdad517b3..b519712d76e 100644 >> --- a/gcc/gimple-ssa-warn-access.cc >> +++ b/gcc/gimple-ssa-warn-access.cc >> @@ -47,7 +47,7 @@ >> #include "tree-object-size.h" >> #include "tree-ssa-strlen.h" >> #include "calls.h" >> -#include "cfgloop.h" >> +#include "cfganal.h" >> #include "intl.h" >> #include "gimple-range.h" >> #include "stringpool.h" >> @@ -4710,6 +4710,9 @@ pass_waccess::execute (function *fun) >> calculate_dominance_info (CDI_DOMINATORS); >> calculate_dominance_info (CDI_POST_DOMINATORS); >> >> + /* Set or clear EDGE_DFS_BACK bits on back edges. */ >> + mark_dfs_back_edges (fun); >> + >> /* Create a new ranger instance and associate it with FUN. */ >> m_ptr_qry.rvals = enable_ranger (fun); >> m_func = fun; > > Jakub >
On Thu, Mar 03, 2022 at 05:08:30PM -0700, Martin Sebor wrote: > > 1) shouldn't it give up for EDGE_ABNORMAL too? I mean, e.g. > > following a non-local goto forced edge from a noreturn call > > to a non-local label (if there is just one) doesn't seem > > right to me > > Possibly yes. I can add it but I don't have a lot of experience with > these bits so if you can suggest a test case to exercise this that > would be helpful. Something like: void foo (void) { __label__ l; __attribute__((noreturn)) void bar (int x) { if (x) goto l; __builtin_trap (); } bar (0); l:; } shows a single EDGE_ABNORMAL from the bar call. But it would need tweaking for the ptr use and clobber. > > 2) if EDGE_DFS_BACK is computed and 1) is done, is there any > > reason why you need 2 levels of protection, i.e. the EDGE_DFS_BACK > > check as well as the visited bitmap (and having them use > > very different answers, if EDGE_DFS_BACK is seen, the function > > will return false, if visited bitmap has a bb, it will return true)? > > Can't the visited bitmap go away? > > Possibly. As I said above, I don't have enough experience with these > bits to make (and test) the changes quickly, or enough bandwidth to > come up to speed on them. Please feel free to make these improvements. I'll change that if it passes testing. Jakub
On 3/2/22 19:15, Martin Sebor wrote: > The -Wdangling-pointer code tests the EDGE_DFS_BACK but the pass never > calls the mark_dfs_back_edges() function that initializes the bit (I > didn't know about it). As a result the bit is not set when expected, > which can cause false positives under the right conditions. > > The attached patch adds a call to the warning pass to initialize > the bit. Tested on x86_64-linux. OK on Monday if no other comments. Jason
Call mark_dfs_back_edges before testing EDGE_DFS_BACK [PR104761]. Resolves: PR middle-end/104761 - bogus -Wdangling-pointer with cleanup and infinite loop gcc/ChangeLog: PR middle-end/104761 * gimple-ssa-warn-access.cc (pass_waccess::execute): Call mark_dfs_back_edges. gcc/testsuite/ChangeLog: PR middle-end/104761 * g++.dg/warn/Wdangling-pointer-4.C: New test. * gcc.dg/Wdangling-pointer-4.c: New test. diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index b7cdad517b3..b519712d76e 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -47,7 +47,7 @@ #include "tree-object-size.h" #include "tree-ssa-strlen.h" #include "calls.h" -#include "cfgloop.h" +#include "cfganal.h" #include "intl.h" #include "gimple-range.h" #include "stringpool.h" @@ -4710,6 +4710,9 @@ pass_waccess::execute (function *fun) calculate_dominance_info (CDI_DOMINATORS); calculate_dominance_info (CDI_POST_DOMINATORS); + /* Set or clear EDGE_DFS_BACK bits on back edges. */ + mark_dfs_back_edges (fun); + /* Create a new ranger instance and associate it with FUN. */ m_ptr_qry.rvals = enable_ranger (fun); m_func = fun; diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C new file mode 100644 index 00000000000..3608cc79e9a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C @@ -0,0 +1,23 @@ +/* PR middle-end/104761 - False positive -Wdangling-pointer warning + in NetworkManager + { dg-do compile } + { dg-options "-O -Wall -fno-exceptions" } */ + +struct S { int i; }; + +struct X { ~X (); }; + +void g (int); + +void test (int i) +{ + S s = { 0 }; + + X x; + + if (i) + { + g (s.i); // { dg-bogus "-Wdangling-pointer" } + for ( ; ; ); + } +} diff --git a/gcc/testsuite/gcc.dg/Wdangling-pointer-4.c b/gcc/testsuite/gcc.dg/Wdangling-pointer-4.c new file mode 100644 index 00000000000..b2716c7b634 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wdangling-pointer-4.c @@ -0,0 +1,23 @@ +/* PR middle-end/104761 - False positive -Wdangling-pointer warning + in NetworkManager + { dg-do compile } + { dg-options "-O -Wall" } */ + +typedef struct { int i; } S; + +void f (S **); + +int g (int); + +void nowarn (int x) +{ + S s = { 0 }; + + __attribute__((__cleanup__ (f))) S *p = 0; + + if (x) + { + g (s.i); // { dg-bogus "-Wdangling-pointer" } + for ( ; ; ); + } +}