[cfe-dev] Working on the rest of PR10063: destructors and the CFG are causing issues with -Wreturn-type

[Chandler Carruth]

So it appears that the remainder of the problems in PR10063 stem from the
following pattern in a CFG:

int f5(int i) {
  other o;
  switch (i) {
    default:
      my_abort_struct();
  }
}

Which has the CFG:
 [ B4 (ENTRY) ]
    Predecessors (0):
    Successors (1): B2

 [ B1 ]
      1: [B2.2].~other() (Implicit destructor)
    Predecessors (1): B3
    Successors (1): B0

 [ B2 ]
      1:
      2: other o;
      3: i
      4: [B2.3]
      T: switch [B2.4]
    Predecessors (1): B4
    Successors (1): B3

 [ B3 ]
    default:
      1: my_abort_struct()
      2: [B3.1] (BindTemporary)
      3: ~my_abort_struct() (Temporary object destructor)
    Predecessors (1): B2
    Successors (1): B1

 [ B0 (EXIT) ]
    Predecessors (1): B1
    Successors (0):

Everything we need is here, but there is an unfortunate structure: the block
(B3) with the no-return destructor is nat an immediate predecessor of the
exit block. As a result, we never look within B3 for no-return destructors,
and fail to recognize that B1 and B0 are dead, and that there is no
possibility of reaching the exit block without returning.

There is a comment in AnalysisBasedWarnings which indicates that the correct
solution may be to actually prevent B1 (or any other block) from being added
as a successor of B3, and explicitly add the exit block as a successor so we
don't end up with an orphaned exit block. Is this the right approach to
take? Can you provide any tips for how to implement this? I looked into it
for a while, and it didn't seem straightforward. I can find all of the
places where we add implicit destructors to the CFG, and when one of them is
added that in attributed no-return, clear the successors of the current
block and add the exit block as a successor. However, it looks like in some
cases we process implicit destructors before actually adding successor(s) to
a block. In that case, clearing won't help. What do you think is the
cleanest place to put this in the CFGBuilder, and what is the cleanest
strategy? Maybe we should track on the CFGBlock itself whether it is a
'no-return' block, and when adding successors check that bit and refuse to
add any. Or we could catch when lazily creating a new block, and if the
previous one contains any no-return elements clear its successors?


I also looked into an alternate solution -- rather than only inspecting the
immediate predecessors of the exit block in -Wreturn-type, we could actually
walk the CFG from the exit block up the graph, stopping along each edge when
we either reach a dead block, a return, or a no-return. This is more
expensive, but a more isolated fix.

Thoughts? I'd really like to get these issues with the CFG fixed as we are
running into more and more problems with this as we start to use CFG-based
warnings and analyses more heavily. How I wish the dominant assertion
mechanism in our codebase didn't use a no-return destructor.... ;]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110910/a9797270/attachment.html>