On 11/10/21 2:27 PM, Marek Polacek wrote:
On Tue, Nov 09, 2021 at 09:28:43PM -0700, Martin Sebor via Gcc-patches wrote:
The attached patch adds support to the middle end for detecting
infinitely recursive calls. The warning is controlled by the new
-Winfinite-recursion option. The option name is the same as
Clang's.
I scheduled the warning pass to run after early inlining to
detect mutually recursive calls but before tail recursion which
turns some recursive calls into infinite loops and so makes
the two indistinguishable.
The warning detects a superset of problems detected by Clang
(based on its tests). It detects the problem in PR88232
(the feature request) as well as the one in PR 87742,
an unrelated problem report that was root-caused to bug due
to infinite recursion.
Nice, I've long wanted this warning. I've made this mistake a couple of
times:
struct S {
operator int() { return S{}; }
};
and the patch warns about it.
Thanks for looking at it! Consider it an early Christmas present :)
Like all middle end warnings, it warns for inline functions only
if their bodies are actually emitted. To handle the others we'd
need to also implement the warning in the front end. Or, "fake"-
emit them somehow as I think Jason was suggesting for
the -Wuninitialized warnings.
I think Clang implements it fully in the font end so it might be
doable at least for the simple subset that doesn't need to traverse
the CFG.
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -629,6 +629,10 @@ Wimplicit-fallthrough=
Common Var(warn_implicit_fallthrough) RejectNegative Joined UInteger Warning
IntegerRange(0, 5)
Warn when a switch case falls through.
+Winfinite-recursion
+Var(warn_infinite_recursion) Warning
+Warn for infinitely recursive calls.
Why do you need this hunk?
So other languages besides the C family can control the warning.
I didn't really think about it too much, but since it's a middle
end warning it seems like they might need to (other than that,
I just copied some other warning that's in both places too).
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ {
+ int eflags = e->flags;
+ if (find_function_exit (fndecl, e->dest, eflags, exit_bb, calls,
visited))
Why not use e->flags directly? find_function_exit can't change it AFAICS.
Only because it's shorter and doesn't break up if () statement
on multiple lines. I think it's easier to read that way. But
in v2 of the patch this is simpler and not necessary anymore.
I find the shadowing of 'loc' unsightly. While here, I think
if (warning_at (DECL_SOURCE_LOCATION (func->decl), OPT_Winfinite_recursion,
"infinite recursion detected"))
for (auto stmt: calls)
{
...
}
would look neater (and avoids the shadowing), but that's just my opinion.
I didn't even notice it but sure.
After thinking about the exceptional handling and taking a closer
look at what Clang warns for I decided that what I had done was
overly conservative and adjusted things to diagnose more of
the same C++ code as it does. I'll post an update shortly,
once it finished testing.
Martin
