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

Alexander Kornienko alexfh at google.com
Mon May 7 17:55:25 PDT 2012


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.

On Mon, May 7, 2012 at 11: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
>

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


More information about the cfe-commits mailing list