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

[David Blaikie]

On Wed, Oct 9, 2013 at 12:27 PM, Richard Trieu <rtrieu at google.com> wrote:

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

If the analysis is applied to template specializations, then, you'd only
get this warning if your code lead to the instantiation of sum<0> - in
which case the warning is correct and useful.

I'm with Arthur - it's still not clear to me why this warning would need to
avoid templates, as long as it ran on template specializations not template
patterns.


>
>> 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
>> >
>>
>
>
> _______________________________________________
> 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/2281ce06/attachment.html>