r202349 - [-Wunreachable-code] Don't warn about unreachable 'default:' cases.

[David Blaikie]

On Thu, Feb 27, 2014 at 1:10 PM, Ted Kremenek <kremenek at apple.com> wrote:

> On Feb 27, 2014, at 11:22 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> I'm not sure I'm entirely following you here (sorry, maybe just
> haven't got enough CFG-related code-state in my head at this point,
> it's been a while since I thought about this). I did notice your
> AdjacentBlock has support for this "alternative" situation you've
> described. When does this come up?
>
>
> Yes, and we have code in there now that uses it.
>
> Here is a specific example:
>
>     noreturn_function();
>     break;
>
> In the pruned CFG, 'noreturn_function()' will be in its own basic block
> and 'break;' in another, and there will not be an edge between them.
>  Indeed, the block with 'noreturn_function()' will have a special block as
> its successor for noreturn function calls.  In ReachableCode.cpp, we look
> to see what would have been the alternate predecessor to 'break;' in the
> CFG, which allows us to determine that it was immediately preceded in the
> original unprunned CFG by a call to this noreturn function.  This is a
> special case we wish to elide as a warning because it adds no value.
>

OK. So the "new" thing here that I hadn't anticipated was the extra edge
for noreturn functions - my original implementation was just that
"unreachable code needs a superset of edges of other analyses", these edges
are an example of that not being true (where the other analyses have edges
that unreachable code doesn't have). Good to know. Thanks for the example.


> We could recover the same kind of information in other ways (e.g.,
> groveling the AST), but they would be very complicated and error prone.
>  Looking at the pure control-flow gives us the information we need.
>

Sure - I totally agreed.


>
>
> Do any analyses need to know about both the reachable and the
> alternative - or is it just the case that any given analysis wants one
> or the other?
>
>
> Most will just care about the reachable blocks.  That said, for successors
> we generally expect them to be laid out in a certain way for some clients
> of the CFG, such as the core static analyzer engine.  I thus see 3 kinds of
> uses:
>
> (1) Clients that just want to *iterate* the reachable
> successor/predecessors.
>
> (2) Clients that want to possibly iterate the successor/predecessors, but
> also want to know *which* successor they are looking at.  For example, if
> we have a block terminated an 'if', we can expect the 'then' block to be
> the first successor and 'else' block to be the second successor.  Perhaps
> we should encode this in a more natural way in the CFG, but that is how it
> is represented now.  Such clients will also be able to see if a given
> branch is infeasible by construction by seeing that the
> successor/predecessor is NULL.  Both -Wuninitialized and ExprEngine rely on
> such details.
>
> (3) Clients that care about the blocks that *would* have been reachable.
>  Currently -Wunreachable-code is the only client of this interface.  For
> such clients, the probably will want to know in some cases which
> successor/predecessor a block was, and not just want to iterate over them
> (which loses this detail).
>

Sorry, I'm not sure I understand your last sentence there - you mean an
unreachable-code (optimistic/unpruned/whatever we want to call it) -like
navigation of the graph, but with the ordering/null gap guarantees of (2)
above? OK.

(hmm - would an unpruned graph ever have those null gaps in the successors?
Or are they only ever used to indicate impossible branches that we've
discovered due to constant folding - well I suppose some constant folding
will still be used in the unpruned graph, just not /certain/ constant
folding (sizeof, dependent expressions, etc))


>
>
> So long as no analysis needs to actually correlate the alternatives -
> ie: so long as they only care about being entirely optimistic or
> entirely pessimistic about edges/reachability, the filtering API still
> seems like a rather clean(er) approach. Though admittedly my initial
> implementation assumed that the set of optimistic edges was a superset
> of the pessimistic edges - if there are alternatives then that's not
> true and I'd need 2 bits (as you've used in AdjacentBlock) rather than
> 1 in the edge to filter on, but that's easily do-able.
>
>
> I think a better (new) starting place would be to identify the problems
> that a revised iterator API would solve.  I don't doubt they exist, but I'd
> rather start from there rather than prescribing it as a solution because
> that's what we came up with before.  The observation that we need 2 bits is
> a revelation I made last night when implementing extra heuristics in
> -Wunreachable-code.  In some ways, I'd rather push the current design a
> little further, flush out more of the design considerations, and then clean
> it up.
>

Sure - if you like. My only concern with doing it this way is that the
incumbent solution tends to look a bit more appealing over time when
looking at how invasive a redesign will be (even now, the extra null checks
that have been inserted are now already there (and won't necessarily be
immediately discoverable if changing to an filtering iterator (they'd just
become always-true tests on pointers)) so the cleanup doesn't look as
significant).

But to list a few of my thoughts in comparison between the two solutions:

* asymmetry between successor and predecessor sets seems
confusing/problematic (but I don't work with these structures often, so I
don't know how problematic that really is - you mentioned in (3) above that
successor unpruned edges might be needed eventually?)

* the explicit querying of alternative edges stored in the same entry seems
like a needless burden on callers (since none seem to care about the fact
that /this/ edge is an alternative to /this/ edge - what they care about is
the optimistic or pessimistic edges) - essentially an "alternate" edge
could be encoded as one unreachable edge and one reachable edge with no
loss of functionality. And that's all the consumers really care about (&
have to care about unreachable, but not alternative, edges - so by having
both kinds it just burdens the unreachable code analysis to consider both
kinds when they're the same thing).

Does that seem reasonable/make sense?

(side thought: actually those null edges in the successor list could
totally use the extra bits to determine if they're null or not
(PointerIntPair?) that way they'd come at no cost - if you navigate for
"optimistic reachability" they're non-null and otherwise they're null - but
that'd just be a minor space optimization compared to putting two edges in,
one optimistic one pessimistic... - hmm, since I didn't have
pessimistic-only edges, I wonder how my filtering iterator solution even
worked for this case... *ponder*)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140227/c6da60c6/attachment.html>