[PATCH] Add new warning to Clang to detect when all code paths in a function has a call back to the function.

[Richard Trieu]

On Tue, Oct 8, 2013 at 9:09 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com>wrote:

> Really cool! I don't understand the template example, though...
>
>     // sum<0>() is instantiated, does recursively call itself, but never
> runs.
>     template <int value> int sum() { return value + sum<value/2>(); }
>  // <== notice that your test is actually missing the (), so you've
> got a bogus syntax error in there
>     template<> int sum<0>() { return 1; }
>

My mistake.  That should be int sum<1>().  For value > 1, it will
eventually call sum<1>().  But if sum<0>() is called, it just calls itself.

>
> You imply that Clang would get confused and think that sum<0> calls
> itself... but how can that be, since sum<0> simply returns 1? Or, if
> you mean that Clang would mistakenly try to instantiate the regular
> ("un-specialized"?) version of sum<i> with i=0... how can *that* be,
> since you could just as well make it
>
>     template <int value> int sum() { static_assert( i != 0, "" );
> return value + sum<value/2>(); }
>
> It would be great if the compiler could only choose to run on good code.
 In practice, we need to look out for edgecases.


> ? Clang shouldn't be trying to static-analyze instantiations that
> aren't real. :P
> I'd like to see this patch fixed so that it works properly on
> templates, because that strikes me as exactly the case where this sort
> of warning would be most useful. We want to be able to distinguish the
> cases
>
>     template <int value> int sum() { return value + sum<value/2>(); }
> // no warning
>     template<> int sum<0>() { return 1; }
>     void f() { sum<4>(); }
>
> and
>
>     template <int value> int sum() { return value + sum<value/2>(); }
> // yes warning
>     void f() { sum<4>(); }
>
>
> Also, FWIW, I see no reason to special-case infinite loops.
>
>     void f() { while (true) f(); }
>
> is buggy enough to deserve a diagnostic, IMO. (Such a construct
> probably never happens in practice, but when it *does* happen, that
> one time in a million, and an engineer spends three hours tracking it
> down, he'll probably be annoyed that Clang specifically considered
> pointing out the problem and then silently swallowed it.)
>

Except that there's plenty of reasons to allow this.  Probably the most
common are programs that should never terminate and have a while(true) loop
in their main function.  It's possible to make a list of the well-known
patterns to ignore [while(1), while(true), for(;;)] and warn on the rest.

>
> my $.02,
> –Arthur
>
>
> On Tue, Oct 8, 2013 at 8:13 PM, Richard Trieu <rtrieu at google.com> wrote:
> > Implement a warning to detect when a function will call itself
> recursively on every code path.  If a program ever calls such a function,
> the function will attempt to call itself until it runs out of stack space.
> >
> > This warning searches the CFG to determine if every codepath results in
> a self call.  In addition to the test for this warning, several other tests
> needed to be fixed, and a pragma to prevent this warning where Clang really
> wants a stack overflow.
> >
> > Testing with this warning has already caught several buggy functions.
>  Common mistakes include: incorrect namespaces, wrapper classes not
> forwarding calls properly, similarly named member function and data member,
> and failing to call an overload of the same function.  When run outside of
> template instantiations, all true positives.  In template instantiations,
> only 25% true positive.  Therefore, this warning is disabled in template
> instantiations.  An example of such a false positive is in the test cases.
> >
> > http://llvm-reviews.chandlerc.com/D1864
> >
> > Files:
> >   test/Analysis/inlining/test-always-inline-size-option.c
> >   test/Analysis/misc-ps-region-store.cpp
> >   test/Analysis/cxx11-crashes.cpp
> >   test/FixIt/typo.m
> >   test/FixIt/fixit.c
> >   test/Sema/unused-expr-system-header.c
> >   test/Sema/warn-unused-function.c
> >   test/Sema/attr-deprecated.c
> >   test/Parser/cxx-using-declaration.cpp
> >   test/Parser/expressions.c
> >   test/CodeGen/functions.c
> >   test/SemaCXX/statements.cpp
> >   test/SemaCXX/warn-bool-conversion.cpp
> >   test/SemaCXX/warn-infinite-recursion.cpp
> >   test/SemaCXX/MicrosoftCompatibility.cpp
> >   test/Lexer/gnu-flags.c
> >   include/clang/Basic/DiagnosticSemaKinds.td
> >   include/clang/Basic/DiagnosticGroups.td
> >   lib/Sema/AnalysisBasedWarnings.cpp
> >   lib/Lex/Pragma.cpp
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131009/321a32e7/attachment.html>