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

[David Blaikie]

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

Hmm - indeed many of my changes (involving iter +1, etc) were about
successor iterators and not predecessor iterators.

That seems confusing to me to allow unreachable blocks to be found via
predecessors but not via successors...

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

Right, that was my intention/design - a filtered iterator, one that
provided the existing view and another that provided the
"optimistically reachable" view.

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

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?

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

I'm not sure what's going to be easier - to review my change in the
absence of yours (I can probably finagle git to locally revert your
changes and rebase my patch on top of those reverts) or as a mutation
to yours. I don't want to hold you up, but think it /might/ be worth
reverting your changes publically to consider the design a bit more.
But whichever way you think will work best for you. Ultimately I
didn't get around to working more on this in 2 years, so I'm certainly
not able to claim ownership/priority for my patch here.

- David