[cfe-commits] [PATCH] Implicit fall-through between switch labels

[Alexander Kornienko]

Hi Richard, David,

On Fri, May 4, 2012 at 12:09 AM, Richard Smith <richard at metafoo.co.uk>
 wrote:

> ...
>
 In testing your patch, I did find one issue. We produce a
>>> 'clang::fallthrough attribute is unreachable' diagnostic for this code:
>>>
>>>   switch (2) {
>>>     case 1:
>>>       f();
>>>       [[clang::fallthrough]];
>>>     case 2:
>>>       break;
>>>   }
>>>
>>> I think we should suppress the diagnostic in this case.
>>>
>> 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?
>>
>
> Because the '2' in the switch statement might come from a macro expansion
> or otherwise be computed. As a random example:
>
> long *p;
> switch (sizeof(*p)) {
> case 8:
>   Write4Bytes(*p >> 32);
>   [[clang::fallthrough]];
> case 4:
>   Write4Bytes(*p);
>   break;
> default:
>   assert("unhandled width");
> }
>
> 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.
>

On Thu, May 3, 2012 at 9:03 PM, David Blaikie <dblaikie at gmail.com> wrote:

> ...
> This is the edge of a large bunch of issues with the current
> unreachable code calculation that you might want to just generally
> steer clear of (by not warning about unreachable fallthroughs that
> would fall through to a case), if possible, until it's improved.
>
> This like:
>
> if (sizeof(int) == 4) { stuff(); [[clang::fallthrough]]; }
>
> will currently be classified as unreachable code, as well as
> expressions using non-type template parameters (or anything much that
> is evaluated at compile-time - macro expansions, etc).
>
> While the particular case Richard has brought up here probably needs a
> special case even if we improve/fix the general unreachable code
> problems (continue to assume the condition expression is
> non-constant/variable for the purposes of this warning in particular)
> I'm not sure whether that'll be sufficient to keep the noise down on
> this warning if you're flagging anything Clang considers unreachable -
> but it might be enough given how narrow these cases are (unreachable
> code based on a compile-time constant condition within a switch's case
> that has a fallthrough attribute).
>
> - David


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:
 1. the easy one: set AC.getCFGBuildOptions().PruneTriviallyFalseEdges to
false in AnalysisBasedWarnings::IssueWarnings - this will probably affect
both performance and correctness of other analysis-based warnings;
 2. unclear one: CFGBuilder's methods tryEvaluate and tryEvaluateBool use
Expr::isTypeDependent() and Expr::isValueDependent() 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.
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.

-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120507/9afb1f5d/attachment.html>