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

[David Blaikie]

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

> On Tue, Nov 12, 2013 at 9:56 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> 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?
>>
> Portion of Clang driver code.
>
>> Is this some randomly-chosen file or is it selected to be the worst case
>> for this warning somehow?
>>
> Randomly chosen.
>
>> 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)?
>>
> I used the standard Clang invocation.  I am not sure if that constructs
> the CFG.  Turning -Wuninitialized on and off produces similar timing
> differences to this warning.
>

The important question is what's the timing difference between:
-Wuninitialized -Wyour-new-warning and just -Wuninitialized

That way you won't be comparing the timing of CFG building and no CFG
building, but CFG building with your warning and CFG building without your
warning.


>
>>
>>> 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
>>>
>>>
>>
>
> _______________________________________________
> 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/458ec91f/attachment.html>