On Tue, Nov 12, 2013 at 10:46 PM, Richard Smith <[email protected]>wrote:
> On Tue, Nov 12, 2013 at 10:37 PM, David Blaikie <[email protected]>wrote: > >> On Tue, Nov 12, 2013 at 10:30 PM, Richard Trieu <[email protected]>wrote: >> >>> On Tue, Nov 12, 2013 at 9:56 PM, Richard Smith <[email protected]>wrote: >>> >>>> On Tue, Nov 12, 2013 at 9:44 PM, Richard Trieu <[email protected]>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 <[email protected]> wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Fri, Nov 8, 2013 at 8:52 PM, Chandler Carruth < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Random thoughts... >>>>>>> >>>>>>> On Fri, Nov 8, 2013 at 3:20 PM, Sean Silva <[email protected]>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 >>>>>>> [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 >>>>> >>>>> >>>> >>> >>> _______________________________________________ >>> 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
