From 24d2e1a4c8cbe3dc18cc71b1726e6c90b3bd0b36 Mon Sep 17 00:00:00 2001 From: Alena Laskavaia Date: Fri, 12 Dec 2014 23:37:42 -0500 Subject: [PATCH] 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 --- .../model/cfg/ControlFlowGraphBuilder.java | 50 +++++++++++++------ .../codan/core/cfg/ControlFlowGraphTest.java | 42 ++++++++++++++++ .../core/cfg/AbstractSingleIncomingNode.java | 6 +-- .../internal/core/cfg/ControlFlowGraph.java | 12 +++-- .../cfgview/views/ControlFlowGraphView.java | 15 ++++-- 5 files changed, 100 insertions(+), 25 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/internal/model/cfg/ControlFlowGraphBuilder.java b/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/internal/model/cfg/ControlFlowGraphBuilder.java index d2f3f5358a8..f737015c20d 100644 --- a/codan/org.eclipse.cdt.codan.core.cxx/src/org/eclipse/cdt/codan/core/cxx/internal/model/cfg/ControlFlowGraphBuilder.java +++ b/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 labels = new HashMap<>(); + Map 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; } diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/cfg/ControlFlowGraphTest.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/cfg/ControlFlowGraphTest.java index dd29b23c73c..d3ff33032de 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/cfg/ControlFlowGraphTest.java +++ b/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 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()); + } } diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/AbstractSingleIncomingNode.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/AbstractSingleIncomingNode.java index 571372cc0ec..c35b1f1f663 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/AbstractSingleIncomingNode.java +++ b/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) { diff --git a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/ControlFlowGraph.java b/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/ControlFlowGraph.java index c6d7003038e..059c83f5a22 100644 --- a/codan/org.eclipse.cdt.codan.core/src/org/eclipse/cdt/codan/internal/core/cfg/ControlFlowGraph.java +++ b/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); + } } } } diff --git a/codan/org.eclipse.cdt.codan.ui.cfgview/src/org/eclipse/cdt/codan/ui/cfgview/views/ControlFlowGraphView.java b/codan/org.eclipse.cdt.codan.ui.cfgview/src/org/eclipse/cdt/codan/ui/cfgview/views/ControlFlowGraphView.java index 7485cee680b..6b722e4722d 100644 --- a/codan/org.eclipse.cdt.codan.ui.cfgview/src/org/eclipse/cdt/codan/ui/cfgview/views/ControlFlowGraphView.java +++ b/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. *

*/ +@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 blocks = getFlat(((IControlFlowGraph) parent).getStartNode(), new ArrayList()); + IControlFlowGraph cfg = (IControlFlowGraph) parent; + Collection blocks = getFlat(cfg.getStartNode(), new ArrayList()); DeadNodes dead = new DeadNodes(); - Iterator iter = ((IControlFlowGraph) parent).getUnconnectedNodeIterator(); + Iterator iter = cfg.getUnconnectedNodeIterator(); for (; iter.hasNext();) { IBasicBlock iBasicBlock = iter.next(); dead.add(iBasicBlock); } - ArrayList all = new ArrayList(); + ArrayList all = new ArrayList(); 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 blocks = new ArrayList(); IBasicBlock[] outgoingNodes = ((IDecisionNode) parent).getOutgoingNodes(); for (int i = 0; i < outgoingNodes.length; i++) { IBasicBlock arc = outgoingNodes[i];