Hi Richard, David,<div><br><div>On Fri, May 4, 2012 at 12:09 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:</div><div><div class="gmail_quote">
<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 class="gmail_quote">... </div></blockquote><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 class="gmail_quote"><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 class="gmail_quote"><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">
<div>In testing your patch, I did find one issue. We produce a 'clang::fallthrough attribute is unreachable' diagnostic for this code:</div><div><br></div><div> switch (2) {</div><div> case 1:</div><div> f();</div>
<div> [[clang::fallthrough]];</div><div> case 2:</div><div> break;</div><div> }</div><div><br></div><div>I think we should suppress the diagnostic in this case.</div></blockquote></div><div>I don't see any issue. the code between case 1 and case 2 is really unreachable. Why would we want to suppress this diagnostic here?</div>
</div></blockquote></div><br><div>Because the '2' in the switch statement might come from a macro expansion or otherwise be computed. As a random example:</div><div><br></div><div>long *p;</div><div>switch (sizeof(*p)) {</div>
<div>case 8:</div><div> Write4Bytes(*p >> 32);</div><div> [[clang::fallthrough]];</div><div>case 4:</div><div> Write4Bytes(*p);</div><div> break;</div><div>default:</div><div> assert("unhandled width");</div>
<div>}</div><div><br></div><div>The purpose of this diagnostic should be to test for the case where the [[clang::fallthrough]]; is unreachable due to every path above it being terminated by 'return', 'break', etc. (where there appears to be a bug in the code, because fallthrough was expected but is impossible), not due to the attribute being unreachable because we've proven that a case label can't be reached.</div>
</blockquote><div> </div></div></div><div class="gmail_quote">On Thu, May 3, 2012 at 9:03 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>...</div>This is the edge of a large bunch of issues with the current<br>
unreachable code calculation that you might want to just generally<br>
steer clear of (by not warning about unreachable fallthroughs that<br>
would fall through to a case), if possible, until it's improved.<br>
<br>
This like:<br>
<br>
if (sizeof(int) == 4) { stuff(); [[clang::fallthrough]]; }<br>
<br>
will currently be classified as unreachable code, as well as<br>
expressions using non-type template parameters (or anything much that<br>
is evaluated at compile-time - macro expansions, etc).<br>
<br>
While the particular case Richard has brought up here probably needs a<br>
special case even if we improve/fix the general unreachable code<br>
problems (continue to assume the condition expression is<br>
non-constant/variable for the purposes of this warning in particular)<br>
I'm not sure whether that'll be sufficient to keep the noise down on<br>
this warning if you're flagging anything Clang considers unreachable -<br>
but it might be enough given how narrow these cases are (unreachable<br>
code based on a compile-time constant condition within a switch's case<br>
that has a fallthrough attribute).<br>
<span><font color="#888888"><br>
- David</font></span></blockquote><div> </div><div>Thank you for explanations, as my imagination in C++ area is still quite limited to a certain subset of features. This kind of cases is definitely handled wrong in current implementation. A range of possible solutions spreads from totally disabling this warning (which is easy, but not too consistent) to implementing correct handling of this category of problems by extending CFG with edges representing statically excluded execution paths. Having done a quick look up, I found a couple of possible ways to do this:</div>
<div> 1. the easy one: set <font face="courier new, monospace">AC.getCFGBuildOptions().PruneTriviallyFalseEdges</font> to <font face="courier new, monospace">false</font> in <font face="courier new, monospace">AnalysisBasedWarnings::IssueWarnings</font> - this will probably affect both performance and correctness of other analysis-based warnings;</div>
<div> 2. unclear one: CFGBuilder's methods <font face="courier new, monospace">tryEvaluate</font> and <font face="courier new, monospace">tryEvaluateBool</font> use <font face="courier new, monospace">Expr::isTypeDependent()</font> and <font face="courier new, monospace">Expr::isValueDependent()</font> methods to detect if the static value of expression depends on template's type or value argument; we can also try to detect other interesting cases similarly: dependence on sizeof, macros etc.</div>
<div>The second way seems better to me. If anyone has more information on which problems this will cause and which edge cases should be considered, I would be glad to hear it.</div>
<div><br></div><div>-- </div><div>Best regards,</div><div>Alexander Kornienko </div></div>
</div>