Bug 455828 - Don't be over-eager when collecting dead nodes in the

control flow graph

Change-Id: I54013e31a197c02698e3161f9f52755e4cb6b203
Signed-off-by: Nathan Ridge <zeratul976@hotmail.com>

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java

@@ -194,12 +194,7 @@ public class ReturnChecker extends AbstractAstFunctionChecker {
 	public Collection<IBasicBlock> getDeadBlocks(IASTFunctionDefinition func) {
 		Collection<IBasicBlock> result = new LinkedHashSet<IBasicBlock>();
 		IControlFlowGraph graph = getModelCache().getControlFlowGraph(func);
-		Iterator<IBasicBlock> unconnectedNodeIterator = graph.getUnconnectedNodeIterator();
+		return ((ControlFlowGraph) graph).getDeadNodes();
-		for (Iterator<IBasicBlock> iterator = unconnectedNodeIterator; iterator.hasNext();) {
-			IBasicBlock block = iterator.next();
-			((ControlFlowGraph) graph).getNodes(block, result);
-		}
-		return result;
 	}
 
 	protected void reportNoRet(IASTFunctionDefinition func, boolean hasRet) {

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/cfg/ControlFlowGraphTest.java

@@ -10,7 +10,6 @@
  *******************************************************************************/
 package org.eclipse.cdt.codan.core.cfg;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Iterator;
 
@@ -562,8 +561,7 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase {
 		IExitNode ret = (IExitNode) startNode.getOutgoing();
 		assertEquals("return 1;", data(ret));
 		IBranchNode labelB = (IBranchNode) graph.getUnconnectedNodeIterator().next(); // BranchNode: b:
-		ArrayList<IBasicBlock> res = new ArrayList<>();
+		Collection<IBasicBlock> res = graph.getDeadNodes();
-		graph.getNodes(labelB, res);
 		assertEquals(6, res.size());
 
 		IJumpNode gotoA = (IJumpNode) ((IConnectorNode) labelB.getOutgoing()).getOutgoing();
@@ -618,6 +616,21 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase {
 		assertEquals("return 1;", data(trueBranch.getOutgoing()));
 	}
 
+	//	int foo(int x) {
+	//	    switch (x) {
+	//	        case 0:
+	//	            return 42;;
+	//	        default:
+	//	    }
+	//	}
+	public void test_dead_statement_in_switch() throws Exception {
+		buildAndCheck(getAboveComment());
+		IDecisionNode swittch = (IDecisionNode) graph.getStartNode().getOutgoing();
+		Collection<IBasicBlock> deadNodes = graph.getDeadNodes();
+		// Make sure the switch statement's merge node has not been marked as dead.
+		assertFalse(deadNodes.contains(swittch.getMergeNode()));
+	}
+
 	//	int main(int a) {
 	//		switch (a) {
 	//			case 1: {

codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java

@@ -448,4 +448,16 @@ public class ReturnCheckerTest extends CheckerTestCase {
 		loadCodeAndRunCpp(getAboveComment());
 		checkErrorLine(1);
 	}
+
+	//	int foo(int x) {
+	//	    switch (x) {
+	//	        case 0:
+	//	            return 42;;
+	//	        default:
+	//	    }
+	//	}
+	public void testDoubleSemicolonInSwitchCase_455828() throws Exception {
+		loadCodeAndRunCpp(getAboveComment());
+		checkErrorLine(1);
+	}
 }

codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/ControlFlowGraph.java

@@ -125,18 +125,11 @@ public class ControlFlowGraph implements IControlFlowGraph {
 	public Collection<IBasicBlock> getNodes() {
 		Collection<IBasicBlock> result = new LinkedHashSet<IBasicBlock>();
 		getNodes(getStartNode(), result);
-		for (Iterator<IBasicBlock> iterator = deadNodes.iterator(); iterator.hasNext();) {
+		getDeadNodes(result);
-			IBasicBlock d = iterator.next();
-			getNodes(d, result);
-		}
 		return result;
 	}
 
-	/**
+	private void getNodes(IBasicBlock start, Collection<IBasicBlock> result) {
-	 * @param d
-	 * @param result
-	 */
-	public void getNodes(IBasicBlock start, Collection<IBasicBlock> result) {
 		if (start == null)
 			return; // huh
 		if (result.contains(start))
@@ -152,4 +145,40 @@ public class ControlFlowGraph implements IControlFlowGraph {
 			}
 		}
 	}
+
+	public Collection<IBasicBlock> getDeadNodes() {
+		Collection<IBasicBlock> result = new LinkedHashSet<IBasicBlock>();
+		getDeadNodes(result);
+		return result;
+	}
+
+	private void getDeadNodes(Collection<IBasicBlock> result) {
+		Collection<IBasicBlock> liveNodes = new LinkedHashSet<IBasicBlock>();
+		getNodes(getStartNode(), liveNodes);
+
+		for (Iterator<IBasicBlock> iterator = deadNodes.iterator(); iterator.hasNext();) {
+			IBasicBlock d = iterator.next();
+			getDeadNodes(d, result, liveNodes);
+		}
+	}
+
+	public void getDeadNodes(IBasicBlock start, Collection<IBasicBlock> result, Collection<IBasicBlock> liveNodes) {
+		if (start == null)
+			return; // huh
+		if (result.contains(start))
+			return;
+		if (liveNodes.contains(start))
+			return;  // a live node is by definition not dead
+		result.add(start);
+		for (IBasicBlock bb : start.getOutgoingNodes()) {
+			getDeadNodes(bb, result, liveNodes);
+		}
+		if (start instanceof IConnectorNode) {
+			// Sometimes, a dead connector node can have incoming nodes that are not otherwise reachable
+			// from unconnected nodes (this happens for a branch node for a dead label).
+			for (IBasicBlock bb : start.getIncomingNodes()) {
+				getDeadNodes(bb, result, liveNodes);
+			}
+		}
+	}
 }