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

[Richard Smith]

On Tue, Nov 12, 2013 at 9:44 PM, Richard Trieu <rtrieu at google.com> wrote:

> Timing details:
>
> 21.03s for Clang
> 21.27s for Patched Clang with warning
>
> This is a difference of .24s or less than 1.5% slowdown.
>
> Times are averaged over 20 runs.  Input is the same pre-processed file.
>  Both Clang binaries were built from the same revision.  -fsyntax-only was
> also used.
>

What's your test case? Is this some randomly-chosen file or is it selected
to be the worst case for this warning somehow? A 1.5% slowdown on average
seems like far too much for this. Was this in a setup where the CFG was
being built regardless (for instance, with -Wuninitialized enabled)?


> On Fri, Nov 8, 2013 at 10:47 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>>
>>
>>
>> On Fri, Nov 8, 2013 at 8:52 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>
>>> Random thoughts...
>>>
>>> On Fri, Nov 8, 2013 at 3:20 PM, Sean Silva <silvas at purdue.edu> wrote:
>>>
>>>> At a higher level, is this really needed as a compiler warning? I mean,
>>>> it's nice and all to detect these things statically, but is this really
>>>> something that needs to be happening on every build?
>>>
>>>
>>> Note that if this is a significant compile time issue, that's relevant.
>>> Everything else is assuming that it isn't. =]
>>>
>>> Could we just do this in the static analyzer? In practice, I can't
>>>> imagine any of these being hard to debug "while developing", since they
>>>> will always result in a stack overflow the second they are called (and then
>>>> you just look at the core file (or the debugger that your IDE attached) to
>>>> see which function it was)
>>>
>>>
>>> Your description alone is the evidence for why developers should have a
>>> warning. ;] First off, stack overflows are notoriously annoying to debug.
>>> Core files are often missing. Running under the debugger is yet another
>>> step to do. I would much rather the compiler just tell me about it.
>>>
>>
>> Absolutely; a static warning is always preferable. All my comments were
>> in light of a perception that this was maybe not cheap enough to justify
>> running at every compile (of course, need to wait for Richard to respond
>> with some real measurements of the compile time impact).
>>
>> -- Sean Silva
>>
>>
>>>
>>> Almost all of our warnings "aren't needed" because they could be tested
>>> and debugged. That doesn't remove their value.
>>>
>>>
>>>> So essentially it seems like this is finding bugs in code that has no
>>>> test coverage and has never been executed in practice; that kind of
>>>> "cleaning out crusty unused parts of the codebase" seems like it would be
>>>> better left to the static analyzer.
>>>
>>>
>>> No, it's shortening the cycle time for developers that hit this bug.
>>>
>>> It also is cleaning out bugs in crufty code, which is always nice. Very
>>> few people will run the static analyzer over all of their crufty code
>>> because if they aren't changing it and it is working in production, they
>>> aren't going to want to sift through the false positives of "maybe that's a
>>> bug". Static analyzers run much more over code under active development.
>>>
>>> And fundamentally, we *routinely* do all of the static analysis that is
>>> sufficiently inexpensive and has sufficiently rare false positives at
>>> compile time. So I think we should focus on those two criteria, the latter
>>> one being the most interesting here.
>>>
>>> _______________________________________________
>>> 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
>>
>>
>
> _______________________________________________
> 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/20131112/2e4a1e1e/attachment.html>