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

Marek Polacek polacek@redhat.com
Thu Nov 11 18:26:57 GMT 2021


On Thu, Nov 11, 2021 at 11:21:01AM -0700, Martin Sebor wrote:
> 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.

Yea, we're probably back at https://gcc.gnu.org/PR21678
 
> 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.

I think so too, but it comes with the usual caveats about implementing
warnings too early/too late.
 
> > > --- 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).
 
It could be marked as Common in common.opt and then you don't need
it in c.opt.  Except you do because you can't do EnabledBy(Wall) in
common.opt :(.

> > > +  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.

Aha, ok.

Marek



More information about the Gcc-patches mailing list