[cfe-commits] [scan-build][PATCH 1/1] Unreachable code checker : false positive in switch statements ?

[Ted Kremenek]

On Feb 28, 2012, at 10:29 PM, David Blaikie <dblaikie at gmail.com> wrote:

> Should we just change the way the CFG is built to include edges for
> default labels on covered switches all the time?

Hi David,

In short, no.  This wouldn't be generally the best solution for all clients.

I think have a better solution (see below).

> That would fix the
> same bug (I'm on the fence about whether it is a bug, myself) in
> Clang's -Wunreachable-code.
> 
> Presumably we're actually doing work/have code designed to check
> covered switches & skip the default edge - we could just remove that
> code I would imagine.
> 
> (one possible problem: that would then mean the default case is
> considered reachable - for reachable-based warnings in
> AnalysisBasedWarnings. Do these default cases fall into the special
> "neither provably reachable nor unreachable" middle ground (like the
> unreachable code in template instantiations that isn't unreachable in
> the pattern, or code that's unreachable on the basis of sizeof
> expressions, macros, etc))

I think I've mentioned before my idea of leaving in "ghost edges" in the CFG that are dead but want to be retained by some clients.  For example, analyses like -Wuninitialized or the static analyzer would largely want to treat ghost edges as being not there for maximum dataflow precision.  Other analyses, such as -Wunreachable-code, may want to include them in their analysis.  Having ghost edges allows us to construct one CFG that can serve multiple clients, and at one extreme allows us to represent both the unoptimized and optimized CFG simultaneously in the same data structure.

Ghost edges would be easy to implement.  Currently CFGBlocks have a vector of successors and predecessors CFGBlock*'s.  Pruned out edges are explicitly NULL CFGBlock pointers.  A ghost edge could be a bitmasked pointer, which could indicate that the edge is dead, but provides extra breadcrumbs for analyses.  We could modify the iterators to handle ghost edges for most clients using the currently observed behavior (i.e., treat all ghost edges as dead), but provide a more general iterator that allows one to interrogate the individual ghost edges when desired.

Getting back to -Wunreachable-code and the static analyzer checker, unfortunately I think just hacking the CFG is not a general solution to solve both of their similar concerns.  While -Wunreachable-code could benefit from ghost edges, the static analyzer path-sensitive engine would first prove that blocks are unreachable by doing its own path-sensitive dataflow analysis on the CFG abstraction that *it* wants to work with.  It is then up the checker to decide whether or not to prune specific warnings.  For those, it could use ghost edges as well, just like -Wunreachable-code, to do its post-process analysis to prune false positives (or rather warnings people don't care about), but the analysis won't be directly the same.  Make sense?

What do you think?

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