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

[Chandler Carruth]

Hey Ted (and any other analyzer hackers out there)

On Sat, Sep 10, 2011 at 1:14 AM, Chandler Carruth <chandlerc at google.com>wrote:

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

So I dived into this with fresh eyes and I think I figured the various bits
of it out. Now I understand better both what was going wrong, and what the
FIXME about severing edges is *really* talking about.

The fundamental problem I'm hitting is that we special case the formation of
CFGBlocks when a no-return construct is encountered. When such a block is
hit, we ignore any successor-sequence already built and instead create a
block with a direct and immediate successor of the exit block. This allows
the analysis to quickly find the no-return CFGBlocks (and elements), and
also ensures that the CFGBlocks that *would* have succeeded a no-return
construct are correctly modeled as unreachable blocks.

In order to handle this situation when working with implicit destructors, we
have to from a new block whenever we encounter one of these destructors, and
correctly setup its successors. I've attached a patch which I think mostly
does this. It adds a FIXME to the only part of the CFG builder where we
don't do this, because that part injects destructors into the *beginning* of
a block. I need to think a bit more about how the block datastructures work
before I'll feel comfortable fixing that, as it'll be a different form of
split from the others. This code path only comes up when patching up gotos,
so I'm not particularly concerned if the no-return modeling there is
imperfect. =]

With this patch I get the following CFG for the code snippet I posted
originally:
 [ B4 (ENTRY) ]
    Predecessors (0):
    Successors (1): B2

 [ B1 ]
      1: [B2.2].~other() (Implicit destructor)
    Predecessors (0):
    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): B0

 [ B0 (EXIT) ]
    Predecessors (2): B1 B3
    Successors (0):

Here we correctly model that B1 cannot be reached, and B3 has the immediate
edge to B0. If I change the code snippet to declare a variable of
my_abort_struct type instead of create a temporary (thus exercising the
other CFG-buiding path I changed in this patch) I get this CFG:
 [ B4 (ENTRY) ]
    Predecessors (0):
    Successors (1): B2

 [ B1 ]
      1: [B2.2].~other() (Implicit destructor)
    Predecessors (0):
    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:
      2: my_abort_struct s;
      3: [B3.2].~my_abort_struct() (Implicit destructor)
    Predecessors (1): B2
    Successors (1): B0

 [ B0 (EXIT) ]
    Predecessors (2): B1 B3
    Successors (0):

Same story, the CFG correctly models the no-return destructor. For a
somewhat trickier case, with 3 variables, the middle forming a no-return
destructor you can even see that this correctly marks which destructor is
unreachable!
code:
int f7(int i) {
  other o;
  switch (i) {
    default:
      other o1;
      my_abort_struct s;
      other o2;
  }
}

CFG:
 [ B5 (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): B5
    Successors (1): B4

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

 [ B4 ]
    default:
      1:
      2: other o1;
      3:
      4: my_abort_struct s;
      5:
      6: other o2;
      7: [B4.6].~other() (Implicit destructor)
      8: [B4.4].~my_abort_struct() (Implicit destructor)
    Predecessors (1): B2
    Successors (1): B0

 [ B0 (EXIT) ]
    Predecessors (2): B1 B4
    Successors (0):


I think this patch is a really useful improvement in accuracy, but it does
add some non-trivial overhead to the process of adding implicit destructors
into the CFG for automatic variables.. Does it seem acceptable? I don't see
particularly cleaner ways of doing this... =/

If this patch is OK, or after any performance tuning you'd like to see here,
I think I also better understand the FIXME in the analysis file. Do you
actually want to switch the CFG to have *no* successors for blocks which
contain a no-return element? That seems like it would be a very clean way of
modelling this and would simplify the analysis for -Wreturn-type
significantly. Let me know your thoughts here; I see a clean way to
implement this after a couple of refactorings to consolidate the code I'm
changing in this patch. (Currently, there is duplicate code. I left the
refactoring and consolidation for a follow-up to make reviewing this one
easier.)

Thanks!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110912/7b5b5111/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-implicit-dtor-noreturn.patch
Type: text/x-patch
Size: 8977 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110912/7b5b5111/attachment.bin>