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

[David Blaikie]

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.

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

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

- David