[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 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?

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