<div dir="ltr">On Tue, Nov 12, 2013 at 10:37 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span style="color:rgb(80,0,80)">On Tue, Nov 12, 2013 at 10:30 PM, Richard Trieu </span><span dir="ltr" style="color:rgb(80,0,80)"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span><span style="color:rgb(80,0,80)"> wrote:</span><br>
<div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>On Tue, Nov 12, 2013 at 9:56 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>On Tue, Nov 12, 2013 at 9:44 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Timing details:<div><br></div><div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif">21.03s for Clang</span></div>
<div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif">21.27s for Patched Clang with warning</span><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif"><br>
</span></div><div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif"><br></span></div><div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif">This is a difference of .24s or less than 1.5% slowdown.</span></div>
<div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif"><br></span></div><div><span style="text-align:right;font-size:13px;font-family:arial,sans,sans-serif">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.</span></div>
</div></blockquote><div><br></div></div><div>What's your test case? </div></div></div></div></blockquote></div><div>Portion of Clang driver code. </div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Is this some randomly-chosen file or is it selected to be the worst case for this warning somehow?</div></div></div></div></blockquote></div><div>Randomly chosen. </div>
<div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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)?</div>
</div></div></div></blockquote></div><div>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.</div></div>
</div></div></blockquote><div><br></div></div><div>The important question is what's the timing difference between: -Wuninitialized -Wyour-new-warning and just -Wuninitialized<br><br>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.</div>
</div></div></div></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div class="gmail_extra"><div class="gmail_quote">On Fri, Nov 8, 2013 at 10:47 PM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Fri, Nov 8, 2013 at 8:52 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">Random thoughts...</div>
<div class="gmail_extra"><br><div class="gmail_quote"><div>
On Fri, Nov 8, 2013 at 3:20 PM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> 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?</blockquote>
<div><br></div></div><div>Note that if this is a significant compile time issue, that's relevant. Everything else is assuming that it isn't. =]</div><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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)</blockquote>
<div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div></div><div>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).</div>
<span><font color="#888888">
<div><br></div><div>-- Sean Silva</div><div> </div></font></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>Almost all of our warnings "aren't needed" because they could be tested and debugged. That doesn't remove their value.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.</blockquote>
</div></div><br>No, it's shortening the cycle time for developers that hit this bug.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
</div>
<br></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div></div></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>