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
