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

[Ted Kremenek]

On Feb 27, 2014, at 10:21 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, Feb 26, 2014 at 10:33 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> On Feb 26, 2014, at 9:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> On Wed, Feb 26, 2014 at 9:42 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> 
>> Author: kremenek
>> Date: Wed Feb 26 23:42:07 2014
>> New Revision: 202349
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=202349&view=rev
>> Log:
>> [-Wunreachable-code] Don't warn about unreachable 'default:' cases.
>> 
>> 
>> I'm not sure this is the right way to model/address this issue. We
>> currently are optimistic about enums only having the enumeration
>> values - which means we'll treat things like defaults and even this:
>> 
>> enum X { A = 1, B = 2};
>> ...
>> switch (some_X) {
>> case A | B: stuff;
>> }
>> 
>> as unreachable, I think - to address both cases, we should be more
>> pessimistic and assume an enum can contain all representable values in
>> the "extra" edges in the CFG.
>> 
>> 
>> Hi David,
> 
> Hi Ted,
> 
> I was about to ask why this ended up off-list... then realized I
> replied off-list to you. Sorry about that - my mistake. Adding
> cfe-commits back in.
> 
>> I completely agree.  What I have done here I hope is just intermediate
>> progress.
>> 
>> My rationale here is that this warning, which has immense potential, hasn't
>> had much traction.  It could have caught the "goto fail" bug fairly easily,
>> if only the warning was on by default.  It seems to me that the warning will
>> reach much higher utility if we can bring it much closer to being on by
>> default (or at least widely deployable), which means possibly trading some
>> true positives for false negatives if we can massively increase the
>> signal-to-noise ratio of this warning.
> 
> I'm totally fine with that tradeoff and agree - getting the false
> positives out of the system (even at the cost of some false negatives)
> is well worth it.
> 
>> For cases like these, we can then
>> refine, allowing the warning to warn in more cases.  I'd rather start by
>> being more conservative about warning, and then open it up.  We've taken
>> that tactic with the static analyzer and it has worked fairly well.
> 
> My point was, in this instance a more general change (to allow all the
> representable values in an enum) would actually remove a whole bunch
> more false positives and is the generalization of the specific change
> you have here. Unless there's some difficulty in doing that (I don't
> think there is, but I could be wrong) I'd prefer we just implement the
> general case here rather than a specific one.

Ah, I see.  Yes that would be fine to do now.

> 
>> It
>> allows us to establish a reasonable baseline, and then make the analysis
>> more aggressive for finding real issues.
> 
> Certainly.
> 
>> 
>> 
>> Speaking of those extra edges - I'll get to reviewing that
>> patch/patches soon. I'd rather like to use the filtering iterator
>> approach I proposed a few years ago (still have the patch lying around
>> somewhere) but haven't taken the time to see how you've solved this &
>> how that compares to what I was working on way back then.
>> 
>> 
>> I'm not married to what I came up with.  Fundamentally the CFG lacked
>> information needed to do this analysis right.  All I did was retrofit back
>> in some of the information the CFG discarded, such as "if this branch was
>> reachable we *would* have had a given block as a successor", etc.  Currently
>> I'm using this for pruning issues.  I've retrofitted this information into
>> the CFG in a way that for the most part preserves the existing invariants
>> that currently clients of the CFG expect.  I mostly just wanted it to be
>> "drop in" at first, with AdjacentBlock auto converting to CFGBlock*.  The
>> main invariant I broke is that the predecessors of a block can now contain
>> null entries (which previously only occurred with successors), but I think
>> there are a handful of cases here and there to fix.
> 
> Right - the solution I proposed (I'll rebase the patch and bump the
> old review thread just to get the context back to the top of the
> stack) was to build that filtering logic into the iterator, rather
> than delegating it to all clients. This way all existing clients
> continue to work as they did before, untouched, which I think is
> important (because there are a bunch of places where those iterators
> are incremented and the null tests you've added aren't necessarily
> pervasive so far as I can tell (judging from the sort of cleanup I had
> to do to enable my change - many cases where code is just doing "iter
> + 1" and dereferencing the result which is would require a loop
> searching for non-null in the scheme you've implemented)).

To be clear, the only client change here was to those looking at the predecessors, which can can see possibly null predecessors.  Existing clients looking at the successors should not be changed at all.  We could change it for both, with some other iterator to see all edges, both reachable and not.

Also, it’s not just a matter of providing a filtered view.  For some successor edges, we the abstraction allows one to encode two plausible edges.  For example, an AdjacentBlock can represent an edge where in the pruned CFG the successor would be one block, but in the unprunned CFG it would be another.  There’s not going to be a single filter that works for clients that care about those differences.  Thus I really only see two general good options: and iterator interface that represents the pruned successors/predecessors, and one that does not (and gives you everything).  Quite frankly, this mostly seems pressing for code looking at predecessors, since we essentially have changed the default behavior here.

> 
>> I'm more than happy for
>> us to clean this up more over time, but I'd rather tackle this with small,
>> incremental progress rather than a (potentially large?) re-architecting of
>> the CFG.
> 
> Agreed - though perhaps we can revisit my original proposal for a
> moment and see whether it's all that large of a change.

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