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

[Ted Kremenek]

I don't have much to add on to David's points, because they are dead on.

CFG construction isn't cheap.  It shows up noticeable under a profile of frontend time, so we don't want to do it multiple times.  Moreover, pruning unreachable branches is a requirement both for the accuracy of various warnings (e.g., -Wuninitialized) and correctness.  In the later case, various warnings are actually pruned from emission if we can tell that they concern code that cannot ever actually execute.

The solution, IMO, is to retain more information in the CFG, not try and generate different permutations of the CFG.  At that point, various builder options such as PruneTriviallyFalseEdges should just disappear, and we should be left with a single CFG that can satisfy the needs of several clients.  That's really our only option, because multiple CFGs for the same function is cost prohibitive from regressing compile time.

There's a backlog of CFG-related changes I'd like to flush out this summer.  I think some of the infrastructure work that David mentions is one of them.  It would unblock -Wunreachable-code, and possibly is essential for this feature as well.  Unfortunately, I can't spend much bandwidth on that until after a certain event in mid-June.

On May 7, 2012, at 2:15 PM, David Blaikie <dblaikie at gmail.com> wrote:

> [+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