From 80c624f2c3bf45de6029e1bc2e2333aa9b241160 Mon Sep 17 00:00:00 2001 From: Nathan Ridge Date: Tue, 17 Feb 2015 02:51:02 -0500 Subject: [PATCH] Bug 455828 - Don't be over-eager when collecting dead nodes in the control flow graph Change-Id: I54013e31a197c02698e3161f9f52755e4cb6b203 Signed-off-by: Nathan Ridge --- .../internal/checkers/ReturnChecker.java | 7 +-- .../codan/core/cfg/ControlFlowGraphTest.java | 19 ++++++-- .../internal/checkers/ReturnCheckerTest.java | 12 +++++ .../internal/core/cfg/ControlFlowGraph.java | 47 +++++++++++++++---- 4 files changed, 67 insertions(+), 18 deletions(-) diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java index 09b955faddf..be4d6be0af8 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ReturnChecker.java +++ b/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 getDeadBlocks(IASTFunctionDefinition func) { Collection result = new LinkedHashSet(); IControlFlowGraph graph = getModelCache().getControlFlowGraph(func); - Iterator unconnectedNodeIterator = graph.getUnconnectedNodeIterator(); - for (Iterator iterator = unconnectedNodeIterator; iterator.hasNext();) { - IBasicBlock block = iterator.next(); - ((ControlFlowGraph) graph).getNodes(block, result); - } - return result; + return ((ControlFlowGraph) graph).getDeadNodes(); } protected void reportNoRet(IASTFunctionDefinition func, boolean hasRet) { 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 baf1b3763b7..1ad638df209 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 @@ -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 res = new ArrayList<>(); - graph.getNodes(labelB, res); + Collection res = graph.getDeadNodes(); 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 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: { diff --git a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java b/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java index f1349c634b5..afdb0ffadaf 100644 --- a/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/ReturnCheckerTest.java +++ b/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); + } } 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 059c83f5a22..53ea9e5c2b8 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 @@ -125,18 +125,11 @@ public class ControlFlowGraph implements IControlFlowGraph { public Collection getNodes() { Collection result = new LinkedHashSet(); getNodes(getStartNode(), result); - for (Iterator iterator = deadNodes.iterator(); iterator.hasNext();) { - IBasicBlock d = iterator.next(); - getNodes(d, result); - } + getDeadNodes(result); return result; } - /** - * @param d - * @param result - */ - public void getNodes(IBasicBlock start, Collection result) { + private void getNodes(IBasicBlock start, Collection result) { if (start == null) return; // huh if (result.contains(start)) @@ -152,4 +145,40 @@ public class ControlFlowGraph implements IControlFlowGraph { } } } + + public Collection getDeadNodes() { + Collection result = new LinkedHashSet(); + getDeadNodes(result); + return result; + } + + private void getDeadNodes(Collection result) { + Collection liveNodes = new LinkedHashSet(); + getNodes(getStartNode(), liveNodes); + + for (Iterator iterator = deadNodes.iterator(); iterator.hasNext();) { + IBasicBlock d = iterator.next(); + getDeadNodes(d, result, liveNodes); + } + } + + public void getDeadNodes(IBasicBlock start, Collection result, Collection 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); + } + } + } }