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

[Richard Trieu]

On Tue, Nov 12, 2013 at 10:46 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Nov 12, 2013 at 10:37 PM, David Blaikie <dblaikie at gmail.com>wrote:
>
>> 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.
>>
>
> Right. If this warning doesn't add measurable compile time in a build
> that's already constructing CFGs, then that's OK, but it needs to be off by
> default, like the other CFG-based warnings.
>
> Using Clang with -Wuninitialized the results fall within the noise on my
system, with some runs with the new warning showing a faster time than
without the new 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/20131113/7bf58a49/attachment.html>