On Wed, Oct 9, 2013 at 1:03 PM, Richard Trieu <[email protected]> wrote:
> > > > On Wed, Oct 9, 2013 at 12:35 PM, David Blaikie <[email protected]> wrote: > >> >> >> >> On Wed, Oct 9, 2013 at 12:27 PM, Richard Trieu <[email protected]> wrote: >> >>> >>> >>> >>> On Tue, Oct 8, 2013 at 9:09 PM, Arthur O'Dwyer < >>> [email protected]> 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 <[email protected]> >>>> 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 >>>> > [email protected] >>>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> > >>>> >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
