[PATCH] Add new warning to Clang to detect when all code paths in a function has a call back to the function.
Richard Trieu
rtrieu at google.com
Wed Oct 9 14:37:38 PDT 2013
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
> >>>>
> >>>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131009/c40571d6/attachment.html>
More information about the cfe-commits
mailing list