[PATCH v2] implement -Winfinite-recursion [PR88232]

[Martin Sebor <msebor@gmail.com>]

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