<div dir="ltr">On Wed, Oct 9, 2013 at 1:33 PM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Ah, Richard's fixed code makes sense (although it's still missing<br>
those function-call parentheses ;)).<br>
I don't think Clang should go out of its way to silence the legitimate<br>
warning about sum<i=0>, though. The fact that its only call *happens*<br>
to be in dead code doesn't mean that we should suppress warnings;<br>
Clang regularly emits diagnostics about code that is dead. And we<br>
certainly shouldn't suppress warnings in *live* template code on the<br>
rationale that well there might be a false positive in dead code<br>
somewhere.<br>
<br>
If any real user is affected by the sum<0> false positive, they can<br>
apply the trivial fix:<br>
<div class="im"><br>
template <int value> int sum() {<br>
</div> return value + (value==0 ? 0 : sum<value/2>()); // the fix is to<br>
add this ?: here<br>
<div class="im">}<br>
template<> int sum<1>() { return 1; }<br>
template<int x, int y> int calculate_value() {<br>
if (x != y)<br>
return sum<x - y>();<br>
else<br>
return 0;<br>
}<br>
int value = calculate_value<1,1>();<br>
<br>
</div>This produces the same machine code as Richard's fixed example,<br>
because the first operand of the ternary operator is a compile-time<br>
constant; yet it avoids the false-positive diagnostic. Meanwhile, we<br>
can allow Clang to diagnose real problems in non-trivial template<br>
code. :)<br>
<span class="HOEnZb"><font color="#888888"><br>
–Arthur<br>
</font></span><div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>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.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
<br>
On Wed, Oct 9, 2013 at 1:09 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Oct 9, 2013 at 1:03 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Wed, Oct 9, 2013 at 12:35 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>><br>
>>><br>
>>> On Wed, Oct 9, 2013 at 12:27 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Tue, Oct 8, 2013 at 9:09 PM, Arthur O'Dwyer<br>
>>>> <<a href="mailto:arthur.j.odwyer@gmail.com">arthur.j.odwyer@gmail.com</a>> wrote:<br>
>>>>><br>
>>>>> Really cool! I don't understand the template example, though...<br>
>>>>><br>
>>>>> // sum<0>() is instantiated, does recursively call itself, but<br>
>>>>> never runs.<br>
>>>>> template <int value> int sum() { return value + sum<value/2>(); }<br>
>>>>> // <== notice that your test is actually missing the (), so you've<br>
>>>>> got a bogus syntax error in there<br>
>>>>> template<> int sum<0>() { return 1; }<br>
>>>><br>
>>>><br>
>>>> My mistake. That should be int sum<1>(). For value > 1, it will<br>
>>>> eventually call sum<1>(). But if sum<0>() is called, it just calls itself.<br>
>>><br>
>>><br>
>>> If the analysis is applied to template specializations, then, you'd only<br>
>>> get this warning if your code lead to the instantiation of sum<0> - in which<br>
>>> case the warning is correct and useful.<br>
>>><br>
>>> I'm with Arthur - it's still not clear to me why this warning would need<br>
>>> to avoid templates, as long as it ran on template specializations not<br>
>>> template patterns.<br>
>>><br>
>><br>
>> Here is the code in question, template parameters fixed now:<br>
>><br>
>> template <int value><br>
>> int sum() {<br>
>> return value + sum<value/2>;<br>
>> }<br>
>><br>
>> template<><br>
>> int sum<1>() { return 1; }<br>
>><br>
>> template<int x, int y><br>
>> int calculate_value() {<br>
>> if (x != y)<br>
>> return sum<x - y>();<br>
>> else<br>
>> return 0;<br>
>> }<br>
>><br>
>> int value = calculate_value<1,1>();<br>
>><br>
>> calculate_value<1,1>() causes the instantiation of sum<0>(), even though<br>
>> it is protected from being called by the conditional "if (x != y)".<br>
>> sum<0>() is the only self-recursive function here. Are you saying we should<br>
>> warn on that even though the programmer added the proper check to prevent it<br>
>> from being called?<br>
><br>
><br>
> OK, thanks for the example.<br>
><br>
> Tricky - I suppose this wouldn't come up if the caller were non-templated,<br>
> because it'd just be dead code. But we can't really track that the only<br>
> instantiation of a template came from dead code (well we certainly don't<br>
> track it now, and it might be a bit much to add such tracking, and certainly<br>
> not immediately).<br>
><br>
> What would happen if we only analyzed functions we codegen? I assume we<br>
> don't codegen that function (due to it being trivially compile-time dead)?<br>
> It would still leave open a small window of false positives in code so<br>
> contrived as to not be demonstrably dead in the frontend...<br>
><br>
> In any case, yes, I accept that it's not simply a matter of "this should<br>
> work for template instantiations" and would require more work to figure out<br>
> how to handle such cases while avoiding false positives - work that can<br>
> happen later if anyone cares to do it/it proves to be sufficiently valuable.<br>
><br>
>>>>><br>
>>>>><br>
>>>>> You imply that Clang would get confused and think that sum<0> calls<br>
>>>>> itself... but how can that be, since sum<0> simply returns 1? Or, if<br>
>>>>> you mean that Clang would mistakenly try to instantiate the regular<br>
>>>>> ("un-specialized"?) version of sum<i> with i=0... how can *that* be,<br>
>>>>> since you could just as well make it<br>
>>>>><br>
>>>>> template <int value> int sum() { static_assert( i != 0, "" );<br>
>>>>> return value + sum<value/2>(); }<br>
>>>>><br>
>>>> It would be great if the compiler could only choose to run on good code.<br>
>>>> In practice, we need to look out for edgecases.<br>
>>>><br>
>>>>><br>
>>>>> ? Clang shouldn't be trying to static-analyze instantiations that<br>
>>>>> aren't real. :P<br>
>>>>> I'd like to see this patch fixed so that it works properly on<br>
>>>>> templates, because that strikes me as exactly the case where this sort<br>
>>>>> of warning would be most useful. We want to be able to distinguish the<br>
>>>>> cases<br>
>>>>><br>
>>>>> template <int value> int sum() { return value + sum<value/2>(); }<br>
>>>>> // no warning<br>
>>>>> template<> int sum<0>() { return 1; }<br>
>>>>> void f() { sum<4>(); }<br>
>>>>><br>
>>>>> and<br>
>>>>><br>
>>>>> template <int value> int sum() { return value + sum<value/2>(); }<br>
>>>>> // yes warning<br>
>>>>> void f() { sum<4>(); }<br>
>>>>><br>
>>>>><br>
>>>>> Also, FWIW, I see no reason to special-case infinite loops.<br>
>>>>><br>
>>>>> void f() { while (true) f(); }<br>
>>>>><br>
>>>>> is buggy enough to deserve a diagnostic, IMO. (Such a construct<br>
>>>>> probably never happens in practice, but when it *does* happen, that<br>
>>>>> one time in a million, and an engineer spends three hours tracking it<br>
>>>>> down, he'll probably be annoyed that Clang specifically considered<br>
>>>>> pointing out the problem and then silently swallowed it.)<br>
>>>><br>
>>>><br>
>>>> Except that there's plenty of reasons to allow this. Probably the most<br>
>>>> common are programs that should never terminate and have a while(true) loop<br>
>>>> in their main function. It's possible to make a list of the well-known<br>
>>>> patterns to ignore [while(1), while(true), for(;;)] and warn on the rest.<br>
>>>>><br>
>>>>><br>
>>>>> my $.02,<br>
>>>>> –Arthur<br>
>>>>><br>
>>>>><br>
>>>>> On Tue, Oct 8, 2013 at 8:13 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>>>>> wrote:<br>
>>>>> > Implement a warning to detect when a function will call itself<br>
>>>>> > recursively on every code path. If a program ever calls such a function,<br>
>>>>> > the function will attempt to call itself until it runs out of stack space.<br>
>>>>> ><br>
>>>>> > This warning searches the CFG to determine if every codepath results<br>
>>>>> > in a self call. In addition to the test for this warning, several other<br>
>>>>> > tests needed to be fixed, and a pragma to prevent this warning where Clang<br>
>>>>> > really wants a stack overflow.<br>
>>>>> ><br>
>>>>> > Testing with this warning has already caught several buggy functions.<br>
>>>>> > Common mistakes include: incorrect namespaces, wrapper classes not<br>
>>>>> > forwarding calls properly, similarly named member function and data member,<br>
>>>>> > and failing to call an overload of the same function. When run outside of<br>
>>>>> > template instantiations, all true positives. In template instantiations,<br>
>>>>> > only 25% true positive. Therefore, this warning is disabled in template<br>
>>>>> > instantiations. An example of such a false positive is in the test cases.<br>
>>>>> ><br>
>>>>> > <a href="http://llvm-reviews.chandlerc.com/D1864" target="_blank">http://llvm-reviews.chandlerc.com/D1864</a><br>
>>>>> ><br>
>>>>> > Files:<br>
>>>>> > test/Analysis/inlining/test-always-inline-size-option.c<br>
>>>>> > test/Analysis/misc-ps-region-store.cpp<br>
>>>>> > test/Analysis/cxx11-crashes.cpp<br>
>>>>> > test/FixIt/typo.m<br>
>>>>> > test/FixIt/fixit.c<br>
>>>>> > test/Sema/unused-expr-system-header.c<br>
>>>>> > test/Sema/warn-unused-function.c<br>
>>>>> > test/Sema/attr-deprecated.c<br>
>>>>> > test/Parser/cxx-using-declaration.cpp<br>
>>>>> > test/Parser/expressions.c<br>
>>>>> > test/CodeGen/functions.c<br>
>>>>> > test/SemaCXX/statements.cpp<br>
>>>>> > test/SemaCXX/warn-bool-conversion.cpp<br>
>>>>> > test/SemaCXX/warn-infinite-recursion.cpp<br>
>>>>> > test/SemaCXX/MicrosoftCompatibility.cpp<br>
>>>>> > test/Lexer/gnu-flags.c<br>
>>>>> > include/clang/Basic/DiagnosticSemaKinds.td<br>
>>>>> > include/clang/Basic/DiagnosticGroups.td<br>
>>>>> > lib/Sema/AnalysisBasedWarnings.cpp<br>
>>>>> > lib/Lex/Pragma.cpp<br>
>>>>> ><br>
>>>>> > _______________________________________________<br>
>>>>> > cfe-commits mailing list<br>
>>>>> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>>>>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>>>> ><br>
>>>><br>
>>>><br>
>>>><br>
>>>> _______________________________________________<br>
>>>> cfe-commits mailing list<br>
>>>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>