Bug 447486 - codan - CCE is thrown by control flow graph builder

- addOutgoing cannot join JumpNode - block will be added to dead list
instead
- addjusted goto code to not use addOutgoing but using setJump directly
- corrected code that doing fake return to deal jump nodes at the end
- corrected code that marks up dead code to remove jump targets
- corrected code that calculates all nodes in the graph to take into
account dangling labels
- fixed CFG viewer to show unconnected labeled statements

Change-Id: Ie4d9e37678e3ebaae8e9f268e6f37342e14a1444
Reviewed-on: https://git.eclipse.org/r/38189
Tested-by: Hudson CI
Reviewed-by: Elena Laskavaia <elaskavaia.cdt@gmail.com>

codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/internal/model/cfg/ControlFlowGraphBuilder.java

@@ -13,7 +13,9 @@ package org.eclipse.cdt.codan.core.cxx.internal.model.cfg;
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
 
 import org.eclipse.cdt.codan.core.cxx.Activator;
 import org.eclipse.cdt.codan.core.model.cfg.IBasicBlock;
@@ -27,6 +29,7 @@ import org.eclipse.cdt.codan.core.model.cfg.IPlainNode;
 import org.eclipse.cdt.codan.core.model.cfg.ISingleOutgoing;
 import org.eclipse.cdt.codan.core.model.cfg.IStartNode;
 import org.eclipse.cdt.codan.internal.core.cfg.AbstractBasicBlock;
+import org.eclipse.cdt.codan.internal.core.cfg.ConnectorNode;
 import org.eclipse.cdt.codan.internal.core.cfg.DecisionNode;
 import org.eclipse.cdt.codan.internal.core.cfg.JumpNode;
 import org.eclipse.cdt.core.dom.ast.IASTBreakStatement;
@@ -71,7 +74,7 @@ public class ControlFlowGraphBuilder {
 	CxxNodeFactory factory = new CxxNodeFactory();
 	IConnectorNode outerBreak;
 	IConnectorNode outerContinue;
-	HashMap<String, IBasicBlock> labels = new HashMap<>();
+	Map<String, IBranchNode> labels = new LinkedHashMap<>();
 
 	/**
 	 * Builds the graph.
@@ -80,9 +83,18 @@ public class ControlFlowGraphBuilder {
 		IASTStatement body = def.getBody();
 		start = new CxxStartNode();
 		exits = new ArrayList<>();
-		dead = new ArrayList<>();
+		dead = new LinkedHashSet<>();
 		IBasicBlock last = createSubGraph(start, body);
-		if (!(last instanceof IExitNode) && !deadConnector(last)) {
+		for (IBranchNode label : labels.values()) {
+			IConnectorNode conn = (IConnectorNode) label.getOutgoing();
+			if (conn.getIncomingSize() <= 1 && label.getIncoming() == null)
+				dead.add(label);
+			else {
+				dead.remove(label);
+				dead.remove(conn);
+			}
+		}
+		if (!(last instanceof IExitNode || last instanceof IJumpNode || deadConnector(last))) {
 			returnExit = factory.createExitNode(null);
 			returnExit.setStartNode(start);
 			addOutgoing(last, returnExit);
@@ -177,14 +189,12 @@ public class ControlFlowGraphBuilder {
 		} else if (body instanceof IASTLabelStatement) {
 			IASTLabelStatement ast = (IASTLabelStatement) body;
 			String labelName = ast.getName().toString();
-			IBranchNode labNode = (IBranchNode) labels.get(labelName);
+			IBranchNode labNode = labels.get(labelName);
 			IConnectorNode conn;
 			if (labNode != null) {
 				conn = (IConnectorNode) labNode.getOutgoing();
 				addOutgoing(prev, labNode);
 			} else {
-				// labeled statement consists of connectors for jumps, branch for
-				// label and nested statement
 				conn = createLabelNodes(prev, labelName);
 			}
 			return createSubGraph(conn, ast.getNestedStatement());
@@ -192,14 +202,16 @@ public class ControlFlowGraphBuilder {
 			IASTGotoStatement ast = (IASTGotoStatement) body;
 			String labelName = ast.getName().toString();
 			IConnectorNode conn;
-			IBranchNode labNode = (IBranchNode) labels.get(labelName);
+			IBranchNode labNode = labels.get(labelName);
 			if (labNode != null) {
 				conn = (IConnectorNode) labNode.getOutgoing();
 			} else {
 				conn = createLabelNodes(null, labelName);
 			}
 			IJumpNode gotoNode = factory.createJumpNode();
-			((JumpNode) gotoNode).setJump(conn, labNode != null);
+			boolean backward = labNode != null; // This is not accurate XXX
+			((JumpNode) gotoNode).setJump(conn, backward);
+			((ConnectorNode) conn).addIncoming(gotoNode);
 			addOutgoing(prev, gotoNode);
 			return gotoNode;
 		} else if (body instanceof IASTProblemStatement) {
@@ -270,9 +282,19 @@ public class ControlFlowGraphBuilder {
 		return node;
 	}
 
+	/**
+	 * labeled statement consists of connector for jumps,
+	 * branch for label and statement
+	 * 
+	 * @param prev
+	 * @param labelName
+	 * @return
+	 */
 	protected IConnectorNode createLabelNodes(IBasicBlock prev, String labelName) {
 		IBranchNode branch = factory.createBranchNode(labelName);
-		if (prev != null)
+		if (prev == null || prev instanceof IJumpNode || prev instanceof IExitNode)
+			;// don't do anything, leave dangling branch node
+		else
 			addOutgoing(prev, branch);
 		labels.put(labelName, branch);
 		IConnectorNode conn = factory.createConnectorNode();
@@ -497,15 +519,15 @@ public class ControlFlowGraphBuilder {
 			return (IJumpNode) prev;
 		if (prev instanceof IExitNode)
 			return null;
-		IJumpNode jump = factory.createJumpNode();
+		JumpNode jump = (JumpNode) factory.createJumpNode();
 		addOutgoing(prev, jump);
-		addOutgoing(jump, conn);
-		((JumpNode) jump).setBackward(backward);
+		jump.setJump(conn, backward);
+		((ConnectorNode) conn).addIncoming(jump);
 		return jump;
 	}
 
 	private void addOutgoing(IBasicBlock prev, IBasicBlock node) {
-		if (prev instanceof IExitNode || prev == null) {
+		if (prev instanceof IExitNode || prev instanceof IJumpNode || prev == null) {
 			dead.add(node);
 			return;
 		}

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

@@ -85,6 +85,8 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase {
 		nodes: for (int i = 0; i < incomingNodes.length; i++) {
 			IBasicBlock b = incomingNodes[i];
 			if (b == null) {
+				if (node instanceof IBranchNode)
+					continue nodes;
 				// check if dead node
 				Iterator<IBasicBlock> iterator = graph.getUnconnectedNodeIterator();
 				boolean dead = false;
@@ -508,4 +510,44 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase {
 		assertSame(conn, jumpEnd(bThen));
 		assertEquals("", data(((IConnectorNode) m2).getOutgoing())); // increment
 	}
+
+//		 int main() {
+//		   goto b;
+//	     a:
+//	      return 2;
+//	     b:
+//	      goto a;
+//		 }
+	public void test_bad_labels() {
+		buildAndCheck(getAboveComment());
+		assertEquals(0, graph.getUnconnectedNodeSize());
+		IStartNode startNode = graph.getStartNode();
+		IJumpNode gotoB =  (IJumpNode) startNode.getOutgoing();
+		IConnectorNode bConn = gotoB.getJumpNode();
+		IJumpNode gotoA =  (IJumpNode) bConn.getOutgoing();
+		IConnectorNode aConn = gotoA.getJumpNode();
+		IExitNode ret =  (IExitNode) aConn.getOutgoing();
+		assertEquals("return 2;", data(ret));
+
+	}
+
+	//	 int main(int a) {
+	//	   goto b;
+	//     if (a)   return 2;
+	//  b:
+	//   return 1;
+	//	 }
+	public void test_labels_if() {
+		buildAndCheck(getAboveComment());
+		assertEquals(1, graph.getUnconnectedNodeSize()); // if is dead code
+	}
+
+	//	 int main(int a) {
+	//	   goto b;
+	//     b:;
+	//	 }
+	public void test_goto() {
+		buildAndCheck(getAboveComment());
+		assertEquals(0, graph.getUnconnectedNodeSize());
+	}
 }

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

@@ -15,7 +15,7 @@ import org.eclipse.cdt.codan.core.model.cfg.ISingleIncoming;
 
 /**
  * Abstract node with one incoming arc (node)
- * 
+ *
  */
 public abstract class AbstractSingleIncomingNode extends AbstractBasicBlock implements ISingleIncoming {
 	private IBasicBlock prev;
@@ -34,7 +34,7 @@ public abstract class AbstractSingleIncomingNode extends AbstractBasicBlock impl
 
 	@Override
 	public int getIncomingSize() {
-		return 1;
+		return prev!=null?1:0;
 	}
 
 	@Override
@@ -44,7 +44,7 @@ public abstract class AbstractSingleIncomingNode extends AbstractBasicBlock impl
 
 	/**
 	 * Sets the incoming node
-	 * 
+	 *
 	 * @param prev
 	 */
 	public void setIncoming(IBasicBlock prev) {

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

@@ -142,10 +142,14 @@ public class ControlFlowGraph implements IControlFlowGraph {
 		if (result.contains(start))
 			return;
 		result.add(start);
-		IBasicBlock[] outgoingNodes = start.getOutgoingNodes();
-		for (int i = 0; i < outgoingNodes.length; i++) {
-			IBasicBlock b = outgoingNodes[i];
-			getNodes(b, result);
+		for (IBasicBlock bb : start.getOutgoingNodes()) {
+			getNodes(bb, result);
+		}
+		if (start instanceof IConnectorNode) {
+			// special case where connect can have some incoming branch nodes not in the graph
+			for (IBasicBlock bb : start.getIncomingNodes()) {
+				getNodes(bb, result);
+			}
 		}
 	}
 }

codan/org.eclipse.cdt.codan.ui.cfgview/src/org/eclipse/cdt/codan/ui/cfgview/views/ControlFlowGraphView.java

@@ -86,6 +86,7 @@ import org.eclipse.ui.texteditor.AbstractTextEditor;
  * are presented in the same way everywhere.
  * <p>
  */
+@SuppressWarnings("restriction")
 public class ControlFlowGraphView extends ViewPart {
 	/**
 	 * The ID of the view as specified by the extension.
@@ -123,20 +124,26 @@ public class ControlFlowGraphView extends ViewPart {
 			if (parent instanceof Collection) {
 				return ((Collection) parent).toArray();
 			} else if (parent instanceof IControlFlowGraph) {
-				Collection<IBasicBlock> blocks = getFlat(((IControlFlowGraph) parent).getStartNode(), new ArrayList<IBasicBlock>());
+				IControlFlowGraph cfg = (IControlFlowGraph) parent;
+				Collection<IBasicBlock> blocks = getFlat(cfg.getStartNode(), new ArrayList<IBasicBlock>());
 				DeadNodes dead = new DeadNodes();
-				Iterator<IBasicBlock> iter = ((IControlFlowGraph) parent).getUnconnectedNodeIterator();
+				Iterator<IBasicBlock> iter = cfg.getUnconnectedNodeIterator();
 				for (; iter.hasNext();) {
 					IBasicBlock iBasicBlock = iter.next();
 					dead.add(iBasicBlock);
 				}
-				ArrayList all = new ArrayList();
+				ArrayList<Object> all = new ArrayList<Object>();
 				all.addAll(blocks);
+				// labeled statements disjoined from the rest
+				for (IBasicBlock node : cfg.getNodes()) {
+					if (node instanceof IBranchNode && node.getIncomingSize() == 0 && !dead.contains(node))
+						all.add(node);
+				}
 				if (dead.size() > 0)
 					all.add(dead);
 				return all.toArray();
 			} else if (parent instanceof IDecisionNode) {
-				ArrayList blocks = new ArrayList();
+				ArrayList<IBasicBlock> blocks = new ArrayList<IBasicBlock>();
 				IBasicBlock[] outgoingNodes = ((IDecisionNode) parent).getOutgoingNodes();
 				for (int i = 0; i < outgoingNodes.length; i++) {
 					IBasicBlock arc = outgoingNodes[i];