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

David Blaikie dblaikie at gmail.com
Wed Oct 9 13:09:17 PDT 2013 

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

>
>
>
> On Wed, Oct 9, 2013 at 12:35 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>>
>> 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.
>>
>>
> Here is the code in question, template parameters fixed now:
>
> template <int value>
> int sum() {
>   return value + sum<value/2>;
> }
>
> template<>
> int sum<1>() { return 1; }
>
> template<int x, int y>
> int calculate_value() {
>   if (x != y)
>     return sum<x - y>();
>   else
>     return 0;
> }
>
> int value = calculate_value<1,1>();
>
> calculate_value<1,1>() causes the instantiation of sum<0>(), even though
> it is protected from being called by the conditional "if (x != y)".
>  sum<0>() is the only self-recursive function here.  Are you saying we
> should warn on that even though the programmer added the proper check to
> prevent it from being called?
>

OK, thanks for the example.

Tricky - I suppose this wouldn't come up if the caller were non-templated,
because it'd just be dead code. But we can't really track that the only
instantiation of a template came from dead code (well we certainly don't
track it now, and it might be a bit much to add such tracking, and
certainly not immediately).

What would happen if we only analyzed functions we codegen? I assume we
don't codegen that function (due to it being trivially compile-time dead)?
It would still leave open a small window of false positives in code so
contrived as to not be demonstrably dead in the frontend...

In any case, yes, I accept that it's not simply a matter of "this should
work for template instantiations" and would require more work to figure out
how to handle such cases while avoiding false positives - work that can
happen later if anyone cares to do it/it proves to be sufficiently valuable.


>
>>>> 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/6e74d44a/attachment.html>

More information about the cfe-commits mailing list