So it appears that the remainder of the problems in PR10063 stem from the following pattern in a CFG:<div><br></div><div><div>int f5(int i) {</div><div>  other o;</div><div>  switch (i) {</div><div>    default:</div><div>      my_abort_struct();</div>
<div>  }</div><div>}</div></div><div><br></div><div>Which has the CFG:</div><div><div> [ B4 (ENTRY) ]</div><div>    Predecessors (0):</div><div>    Successors (1): B2</div><div> </div><div> [ B1 ]</div><div>      1: [B2.2].~other() (Implicit destructor)</div>
<div>    Predecessors (1): B3</div><div>    Successors (1): B0</div><div> </div><div> [ B2 ]</div><div>      1:</div><div>      2: other o;</div><div>      3: i</div><div>      4: [B2.3]</div><div>      T: switch [B2.4]</div>
<div>    Predecessors (1): B4</div><div>    Successors (1): B3</div><div> </div><div> [ B3 ]</div><div>    default:</div><div>      1: my_abort_struct()</div><div>      2: [B3.1] (BindTemporary)</div><div>      3: ~my_abort_struct() (Temporary object destructor)</div>
<div>    Predecessors (1): B2</div><div>    Successors (1): B1</div><div> </div><div> [ B0 (EXIT) ]</div><div>    Predecessors (1): B1</div><div>    Successors (0):</div></div><div><br></div><div>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.</div>
<div><br></div><div>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?</div>
<div><br></div><div><br></div><div>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.</div>
<div><br></div><div>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.... ;]</div>