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

[Arthur O'Dwyer]

Well, I do think that you should run this diagnostic over a few real
codebases (libc++? :)) and see whether it produces any false
positives, first. I suspect there won't be any cases in which the
diagnostic is produced on code that an impartial observer wouldn't
consider inelegant.

Remember, the goal is to catch bugs involving accidental infinite
recursion (due to human confusion with name lookup, overloading,
template instantiation, or parameter pack expansion). If we silence
the diagnostics on templates, then we really limit the usefulness of
this diagnostic.

Can you give some examples of real-world code (A) where this
diagnostic found bugs in non-template code, (B) where this diagnostic
gave false positives on non-template code, (C) where this diagnostic
gave false positives on template code, and (D) where this diagnostic
found bugs in template code?

Thanks,
–Arthur

On Wed, Oct 9, 2013 at 2:37 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Wed, Oct 9, 2013 at 1:33 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com>
> wrote:
>>
>> Ah, Richard's fixed code makes sense (although it's still missing
>> those function-call parentheses ;)).
>> I don't think Clang should go out of its way to silence the legitimate
>> warning about sum<i=0>, though. The fact that its only call *happens*
>> to be in dead code doesn't mean that we should suppress warnings;
>> Clang regularly emits diagnostics about code that is dead. And we
>> certainly shouldn't suppress warnings in *live* template code on the
>> rationale that well there might be a false positive in dead code
>> somewhere.
>>
>> If any real user is affected by the sum<0> false positive, they can
>> apply the trivial fix:
>>
>> template <int value> int sum() {
>>     return value + (value==0 ? 0 : sum<value/2>());  // the fix is to
>> add this ?: here
>> }
>> 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>();
>>
>> This produces the same machine code as Richard's fixed example,
>> because the first operand of the ternary operator is a compile-time
>> constant; yet it avoids the false-positive diagnostic. Meanwhile, we
>> can allow Clang to diagnose real problems in non-trivial template
>> code. :)
>>
>> –Arthur
>>
> While you may be able to figure out a simple way to avoid triggering this
> warning for this test case, not all cases can be reduced to a nice fix-it.
> In the general case, Clang can't distinguish between live or dead code.
> What should the diagnostic and/or fix-it be?  It's bad if we warn on good
> code, and worse if we suggest a bad fix-it.
>>
>>
>> On Wed, Oct 9, 2013 at 1:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> >
>> > 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
>> >>>>
>> >>>
>> >>
>> >
>
>