From 3cf5ec86eef49327bd52622b702ca65c6af68472 Mon Sep 17 00:00:00 2001 From: Alena Laskavaia Date: Tue, 17 Feb 2015 22:12:50 -0500 Subject: [PATCH] Bug 327375 control flow graph regression - dead connector nodes basically for the code like if (a) return 1; else return 2; a++; // this should be a dead code node but we don't check connector node of "if" and don't add it to dead nodes graph resulting it this code not being marked as dead, which can lead to false positives or false negatives for checkers that use that. Same apply for other control statements... Change-Id: Iafb4b48ca960f0ffab2c0265285a31cb08eb0d11 --- .../model/cfg/ControlFlowGraphBuilder.java | 41 +++++-- .../codan/core/cfg/ControlFlowGraphTest.java | 115 ++++++++++++++---- .../internal/checkers/ReturnCheckerTest.java | 14 +++ 3 files changed, 135 insertions(+), 35 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 07aea511da1..8e3094593a7 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 @@ -312,16 +312,23 @@ public class ControlFlowGraphBuilder { addOutgoing(ifNode, elseNode); IBasicBlock els = createSubGraph(elseNode, body.getElseClause()); addJump(els, mergeNode); + fixConnector(mergeNode); return mergeNode; } + protected void fixConnector(IConnectorNode mergeNode) { + if (mergeNode.getIncomingSize()==0) + dead.add(mergeNode); // dead connector node + } + private IBasicBlock createSwitch(IBasicBlock prev, IASTSwitchStatement body) { DecisionNode node = factory.createDecisionNode(body.getControllerExpression()); addOutgoing(prev, node); - IConnectorNode conn = factory.createConnectorNode(); - node.setMergeNode(conn); - createSwitchBody(node, conn, body.getBody()); - return conn; + IConnectorNode mergeNode = factory.createConnectorNode(); + node.setMergeNode(mergeNode); + createSwitchBody(node, mergeNode, body.getBody()); + fixConnector(mergeNode); + return mergeNode; } private void createSwitchBody(IDecisionNode switchNode, IConnectorNode mergeNode, IASTStatement body) { @@ -380,9 +387,13 @@ public class ControlFlowGraphBuilder { IConnectorNode savedBreak = outerBreak; outerContinue = nContinue; outerBreak = nBreak; - IBasicBlock endBody = createSubGraph(loopStart, forNode.getBody()); - outerContinue = savedContinue; - outerBreak = savedBreak; + IBasicBlock endBody = decision; + try { + endBody = createSubGraph(loopStart, forNode.getBody()); + } finally { + outerContinue = savedContinue; + outerBreak = savedBreak; + } // inc IPlainNode inc = factory.createPlainNode(forNode.getIterationExpression()); addOutgoing(endBody, nContinue); @@ -393,6 +404,7 @@ public class ControlFlowGraphBuilder { IBranchNode loopEnd = factory.createBranchNode(IBranchNode.ELSE); addOutgoing(decision, loopEnd); addJump(loopEnd, nBreak); + fixConnector(nBreak); return nBreak; } @@ -432,6 +444,7 @@ public class ControlFlowGraphBuilder { IBranchNode loopEnd = factory.createBranchNode(IBranchNode.ELSE); addOutgoing(decision, loopEnd); addJump(loopEnd, nBreak); + fixConnector(nBreak); return nBreak; } @@ -453,16 +466,21 @@ public class ControlFlowGraphBuilder { IConnectorNode savedBreak = outerBreak; outerContinue = nContinue; outerBreak = nBreak; - IBasicBlock endBody = createSubGraph(loopStart, body.getBody()); - // Restore - outerContinue = savedContinue; - outerBreak = savedBreak; + IBasicBlock endBody = decision; + try { + endBody = createSubGraph(loopStart, body.getBody()); + } finally { + // Restore + outerContinue = savedContinue; + outerBreak = savedBreak; + } // Backward jump addJump(endBody, nContinue, true); // Connect with else branch IBranchNode loopEnd = factory.createBranchNode(IBranchNode.ELSE); addOutgoing(decision, loopEnd); addJump(loopEnd, nBreak); + fixConnector(nBreak); return nBreak; } @@ -500,6 +518,7 @@ public class ControlFlowGraphBuilder { // Add break connector decision.setMergeNode(nBreak); addJump(loopEnd, nBreak); + fixConnector(nBreak); return nBreak; } 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 d3ff33032de..6a909523270 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,6 +10,7 @@ *******************************************************************************/ package org.eclipse.cdt.codan.core.cfg; +import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; @@ -34,7 +35,7 @@ import org.eclipse.cdt.core.dom.ast.c.CASTVisitor; import org.eclipse.cdt.core.parser.ParserLanguage; /** - * TODO: add description + * Tests for ControlFlowGraph */ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { ControlFlowGraph graph; @@ -111,7 +112,7 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { fail("Block " + node + " inconsitent next/prev " + b); } if (node instanceof IDecisionNode && decision) { - assertTrue("decision node outgoing size " + node.getOutgoingSize(), node.getOutgoingSize() > 1); + assertTrue("decision node outgoing size " + node.getOutgoingSize(), node.getOutgoingSize() >= 1); assertNotNull(((IDecisionNode) node).getMergeNode()); } } @@ -159,10 +160,12 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { parse(code, ParserLanguage.C, true); processAst(tu); } + private void buildCfg_CPP(String code) { parse(code, ParserLanguage.CPP, true); processAst(tu); } + /** * @param des * @return @@ -434,17 +437,16 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { IBasicBlock bElse = branchEnd(des, IBranchNode.ELSE); IBasicBlock m2 = jumpEnd(branchEnd(des, IBranchNode.THEN)); IBasicBlock m1 = jumpEnd(bElse); - assertNull(m2); assertNotNull(m1); } -// int test1_f() -// { -// while (1) -// { -// } -// } + // int test1_f() + // { + // while (1) + // { + // } + // } public void test_infiniloop() { buildCfg_C(getAboveComment()); checkCfg(false); @@ -456,7 +458,6 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { IBasicBlock bThen = branchEnd(des, IBranchNode.THEN); IBasicBlock m2 = jumpEnd(bThen); IBasicBlock m1 = jumpEnd(bElse); - assertNotNull(m2); assertNull(m1); } @@ -469,7 +470,6 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { public void test_for() { buildCfg_CPP(getAboveComment()); checkCfg(false); - IStartNode startNode = graph.getStartNode(); IPlainNode decl = (IPlainNode) startNode.getOutgoing(); IConnectorNode conn = (IConnectorNode) decl.getOutgoing(); @@ -478,7 +478,6 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { IPlainNode bThen = (IPlainNode) branchEnd(des, IBranchNode.THEN); assertEquals("bar();", data(bThen)); IBasicBlock bElse = branchEnd(des, IBranchNode.ELSE); - IBasicBlock m1 = jumpEnd(bElse); IBasicBlock m2 = bThen.getOutgoing(); assertNotNull(m1); @@ -494,7 +493,6 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { public void test_range_loop() { buildCfg_CPP(getAboveComment()); checkCfg(false); - IStartNode startNode = graph.getStartNode(); IPlainNode decl = (IPlainNode) startNode.getOutgoing(); IConnectorNode conn = (IConnectorNode) decl.getOutgoing(); @@ -511,24 +509,23 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { assertEquals("", data(((IConnectorNode) m2).getOutgoing())); // increment } -// int main() { -// goto b; -// a: -// return 2; -// b: -// goto a; -// } + // 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(); + IJumpNode gotoB = (IJumpNode) startNode.getOutgoing(); IConnectorNode bConn = gotoB.getJumpNode(); - IJumpNode gotoA = (IJumpNode) bConn.getOutgoing(); + IJumpNode gotoA = (IJumpNode) bConn.getOutgoing(); IConnectorNode aConn = gotoA.getJumpNode(); - IExitNode ret = (IExitNode) aConn.getOutgoing(); + IExitNode ret = (IExitNode) aConn.getOutgoing(); assertEquals("return 2;", data(ret)); - } // int main(int a) { @@ -550,4 +547,74 @@ public class ControlFlowGraphTest extends CodanFastCxxAstTestCase { buildAndCheck(getAboveComment()); assertEquals(0, graph.getUnconnectedNodeSize()); } + + // int main() { + // return 1; + // a: + // return 2; + // b: + // goto a; + // } + public void test_dead_labels() { + buildAndCheck(getAboveComment()); + assertEquals(1, graph.getUnconnectedNodeSize()); + IStartNode startNode = graph.getStartNode(); + 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); + assertEquals(6, res.size()); + + IJumpNode gotoA = (IJumpNode) ((IConnectorNode) labelB.getOutgoing()).getOutgoing(); + IConnectorNode aConn = gotoA.getJumpNode(); + IExitNode ret2 = (IExitNode) aConn.getOutgoing(); + assertEquals("return 2;", data(ret2)); + assertTrue(res.contains(gotoA));// goto a; + assertTrue(res.contains(aConn)); + assertTrue(res.contains(ret2)); // return 2; + assertTrue(res.contains(ret2.getIncoming())); // Branch Node: a: + } + + // int main(int a) { + // if (a) { + // return 1; + // } else { + // return 2; + // } + // a++; + // } + public void test_dead_connector() { + buildAndCheck(getAboveComment()); + assertEquals(1, graph.getUnconnectedNodeSize()); + IConnectorNode connIf = (IConnectorNode) graph.getUnconnectedNodeIterator().next(); + assertEquals("a++;", data(connIf.getOutgoing())); + } + + // int main(int a) { + // for (;1;a++) { + // return 1; + // } + // a--; + // } + public void test_dead_connector_for() { + buildAndCheck(getAboveComment()); + assertEquals(2, graph.getUnconnectedNodeSize()); + IConnectorNode conn = (IConnectorNode) graph.getUnconnectedNodeIterator().next(); + assertEquals("a++", data(conn.getOutgoing())); + } + + // int main(int a) { + // while (0) { + // return 1; + // } + // + // a++; + // } + public void test_dead_connector_while() { + buildAndCheck(getAboveComment()); + assertEquals(1, graph.getUnconnectedNodeSize()); + IBranchNode trueBranch = (IBranchNode) graph.getUnconnectedNodeIterator().next(); + assertEquals("return 1;", data(trueBranch.getOutgoing())); + } } 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 049b5ee2348..a5b01093749 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 @@ -223,6 +223,20 @@ public class ReturnCheckerTest extends CheckerTestCase { loadCodeAndRunCpp(getAboveComment()); checkNoErrors(); } + + //int bar(int foo) + //{ + // if(foo) + // return 0; + // else + // return 0; + // foo++; + //} + public void testBranchesDeadCode_Bug343767() { + loadCodeAndRunCpp(getAboveComment()); + checkNoErrors(); + } + // int f() // { // switch (g()) {