[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
arthur.j.odwyer at gmail.com
Wed Oct 9 13:33:42 PDT 2013
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
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
>>>>
>>>
>>
>
More information about the cfe-commits
mailing list