<div>David, thank you for explanation. I read related topics and got the idea, but if you already started with adding 'ghost edges' or any analogous information to CFG, I would probably not add much value if I start it once again. So I would rather wait until you finish with this and Ted will have time to review this code. Once it happens, I would be happy to fix the code in diagnostics to use proper information from CFG.</div>
<div><br></div><div class="gmail_quote">On Mon, May 7, 2012 at 11:15 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">
[+Ted Kremenek, since this gets into CFG design issues that he has a<br>
vested interest in/ownership over]<br>
<div><div class="h5"><br>
On Mon, May 7, 2012 at 1:56 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
> Hi Richard, David,<br>
><br>
> On Fri, May 4, 2012 at 12:09 AM, Richard<br>
> Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> ...<br>
>>>><br>
>>>> In testing your patch, I did find one issue. We produce a<br>
>>>> 'clang::fallthrough attribute is unreachable' diagnostic for this code:<br>
>>>><br>
>>>> switch (2) {<br>
>>>> case 1:<br>
>>>> f();<br>
>>>> [[clang::fallthrough]];<br>
>>>> case 2:<br>
>>>> break;<br>
>>>> }<br>
>>>><br>
>>>> I think we should suppress the diagnostic in this case.<br>
>>><br>
>>> I don't see any issue. the code between case 1 and case 2 is really<br>
>>> unreachable. Why would we want to suppress this diagnostic here?<br>
>><br>
>><br>
>> Because the '2' in the switch statement might come from a macro expansion<br>
>> or otherwise be computed. As a random example:<br>
>><br>
>> long *p;<br>
>> switch (sizeof(*p)) {<br>
>> case 8:<br>
>> Write4Bytes(*p >> 32);<br>
>> [[clang::fallthrough]];<br>
>> case 4:<br>
>> Write4Bytes(*p);<br>
>> break;<br>
>> default:<br>
>> assert("unhandled width");<br>
>> }<br>
>><br>
>> The purpose of this diagnostic should be to test for the case where the<br>
>> [[clang::fallthrough]]; is unreachable due to every path above it being<br>
>> terminated by 'return', 'break', etc. (where there appears to be a bug in<br>
>> the code, because fallthrough was expected but is impossible), not due to<br>
>> the attribute being unreachable because we've proven that a case label can't<br>
>> be reached.<br>
><br>
><br>
> On Thu, May 3, 2012 at 9:03 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> ...<br>
>> 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>
>><br>
>> - David<br>
><br>
><br>
> Thank you for explanations, as my imagination in C++ area is still quite<br>
> limited to a certain subset of features. This kind of cases is definitely<br>
> handled wrong in current implementation. A range of possible solutions<br>
> spreads from totally disabling this warning (which is easy, but not too<br>
> consistent) to implementing correct handling of this category of problems by<br>
> extending CFG with edges representing statically excluded execution paths.<br>
> Having done a quick look up, I found a couple of possible ways to do this:<br>
> 1. the easy one: set AC.getCFGBuildOptions().PruneTriviallyFalseEdges to<br>
> false in AnalysisBasedWarnings::IssueWarnings - this will probably affect<br>
> both performance<br>
<br>
</div></div>If you don't actually have any need to flag unreachable code<br>
especially, this might not be so bad (though I (& I suspect others)<br>
would probably prefer this CFG to be built once - with both trivially<br>
false edges and their absence)<br>
<div class="im"><br>
> and correctness of other analysis-based warnings;<br>
<br>
</div>Right - to address this we'd need either two CFGs or, probably<br>
preferably, one CFG with both kinds of edges (which clients can filter<br>
out as required).<br>
<div class="im"><br>
> 2. unclear one: CFGBuilder's methods tryEvaluate and<br>
> tryEvaluateBool use Expr::isTypeDependent() and Expr::isValueDependent()<br>
> methods to detect if the static value of expression depends on template's<br>
> type or value argument; we can also try to detect other interesting cases<br>
> similarly: dependence on sizeof, macros etc.<br>
> The second way seems better to me. If anyone has more information on which<br>
> problems this will cause and which edge cases should be considered, I would<br>
> be glad to hear it.<br>
<br>
</div>This is something that's already desired (as I mentioned, to improve<br>
the unreachable code warning) but might be a bigger puzzle than you<br>
want to solve for this immediate issue (I'm not sure - perhaps this is<br>
something that's in scope/interesting to you). I've started work on<br>
this (see this thread:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-March/020234.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-March/020234.html</a> (the<br>
archive is a bit patchy, since it's grouped by month - so you can<br>
search around to find the full email thread history - let me know if<br>
you have trouble finding all the bits)). At the moment I'm trying to<br>
refactor some existing filtering iterator code in Clang (see my recent<br>
patch <a href="http://permalink.gmane.org/gmane.comp.compilers.clang.scm/50765" target="_blank">http://permalink.gmane.org/gmane.comp.compilers.clang.scm/50765</a><br>
) before integrating those more general filtering/adapting iterator<br>
adapters/decorators for use in my improvements to the CFG to track<br>
more than one kind of edge. I certainly don't mind the help/opinions -<br>
nor would I hold it against you if you took another approach/tangent.<br>
<br>
The solution to templates is actually a little different, and actually<br>
simpler: simply do unreachable code analysis on template patterns<br>
rather than template specializations. The template itself is already<br>
conservative about constant evaluations - because it doesn't know how<br>
to evaluate dependent expressions.<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span></blockquote></div><br>--
<div>Best regards,</div><div>Alexander Kornienko</div>