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

[Ted Kremenek]

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.

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.

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140227/3644d3c9/attachment.html>