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

[David Blaikie]

[+Ted Kremenek, since this gets into CFG design issues that he has a
vested interest in/ownership over]

On Mon, May 7, 2012 at 1:56 PM, Alexander Kornienko <alexfh at google.com> wrote:
> 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

If you don't actually have any need to flag unreachable code
especially, this might not be so bad (though I (& I suspect others)
would probably prefer this CFG to be built once - with both trivially
false edges and their absence)

>  and correctness of other analysis-based warnings;

Right - to address this we'd need either two CFGs or, probably
preferably, one CFG with both kinds of edges (which clients can filter
out as required).

>  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.

This is something that's already desired (as I mentioned, to improve
the unreachable code warning) but might be a bigger puzzle than you
want to solve for this immediate issue (I'm not sure - perhaps this is
something that's in scope/interesting to you). I've started work on
this (see this thread:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-March/020234.html (the
archive is a bit patchy, since it's grouped by month - so you can
search around to find the full email thread history - let me know if
you have trouble finding all the bits)). At the moment I'm trying to
refactor some existing filtering iterator code in Clang (see my recent
patch http://permalink.gmane.org/gmane.comp.compilers.clang.scm/50765
) before integrating those more general filtering/adapting iterator
adapters/decorators for use in my improvements to the CFG to track
more than one kind of edge. I certainly don't mind the help/opinions -
nor would I hold it against you if you took another approach/tangent.

The solution to templates is actually a little different, and actually
simpler: simply do unreachable code analysis on template patterns
rather than template specializations. The template itself is already
conservative about constant evaluations - because it doesn't know how
to evaluate dependent expressions.

- David