From a428c0200753e87f7355f1022b69a10555b9ad88 Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Tue, 10 Oct 2023 13:02:18 +0200 Subject: [PATCH] improve `removeEdge` - doesn't always remove all edges when removing at the tail - returns a boolean indicating if an edge was removed --- .../sootup/core/graph/MutableBasicBlock.java | 8 +- .../core/graph/MutableBlockStmtGraph.java | 81 ++++++------------- .../sootup/core/graph/MutableStmtGraph.java | 8 +- .../core/graph/MutableBlockStmtGraphTest.java | 18 ++--- 4 files changed, 44 insertions(+), 71 deletions(-) diff --git a/sootup.core/src/main/java/sootup/core/graph/MutableBasicBlock.java b/sootup.core/src/main/java/sootup/core/graph/MutableBasicBlock.java index 3a06ccdab49..5169dc306f7 100644 --- a/sootup.core/src/main/java/sootup/core/graph/MutableBasicBlock.java +++ b/sootup.core/src/main/java/sootup/core/graph/MutableBasicBlock.java @@ -82,12 +82,12 @@ public void addSuccessorBlock(@Nonnull MutableBasicBlock block) { successorBlocks.add(block); } - public void removePredecessorBlock(@Nonnull MutableBasicBlock b) { - predecessorBlocks.remove(b); + public boolean removePredecessorBlock(@Nonnull MutableBasicBlock b) { + return predecessorBlocks.remove(b); } - public void removeSuccessorBlock(@Nonnull MutableBasicBlock b) { - successorBlocks.remove(b); + public boolean removeSuccessorBlock(@Nonnull MutableBasicBlock b) { + return successorBlocks.remove(b); } public void addExceptionalSuccessorBlock(@Nonnull ClassType exception, MutableBasicBlock b) { diff --git a/sootup.core/src/main/java/sootup/core/graph/MutableBlockStmtGraph.java b/sootup.core/src/main/java/sootup/core/graph/MutableBlockStmtGraph.java index 5039e1a81c2..24e2b07cbbe 100644 --- a/sootup.core/src/main/java/sootup/core/graph/MutableBlockStmtGraph.java +++ b/sootup.core/src/main/java/sootup/core/graph/MutableBlockStmtGraph.java @@ -958,20 +958,15 @@ private void linkBlocks(@Nonnull MutableBasicBlock blockA, @Nonnull MutableBasic } @Override - public void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) { - // FIXME: how to handle "partial" removals of targets of flows starting from a Branching Stmt.. - // e.g. because one of the targets are removed.. that changes the whole logic there.. - + public boolean removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) { MutableBasicBlock blockOfFrom = stmtToBlock.get(from); MutableBasicBlock blockOfTo = stmtToBlock.get(to); if (blockOfFrom == null || blockOfTo == null) { // one of the Stmts is not existing anymore in this graph - so neither a connection. - return; + return false; } - removeBlockBorderEdgesInternal(from, blockOfFrom); - // divide block if from and to are from the same block if (blockOfFrom == blockOfTo) { // divide block and don't link them @@ -985,61 +980,35 @@ public void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) { blockOfFrom.clearSuccessorBlocks(); blocks.add(newBlock); newBlock.getStmts().forEach(s -> stmtToBlock.put(s, newBlock)); + return true; } else { - // throw new IllegalArgumentException("Can't seperate the flow from '"+from+"' to '"+to+"'. - // The Stmts are not connected in this graph!"); + // `from` and `to` are not successive statements in the block + return false; } - } - } + } else { + // `from` and `to` are part of different blocks - protected void removeBlockBorderEdgesInternal( - @Nonnull Stmt from, @Nonnull MutableBasicBlock blockOfFrom) { - // TODO: is it intuitive to remove connections to the BasicBlock in the case we cant merge the - // blocks? - // TODO: reuse tryMerge*Block? - - // add BlockB to BlockA if blockA has no branchingstmt as tail && same traps - if (!blockOfFrom.getStmts().isEmpty() && from == blockOfFrom.getTail()) { - if (blockOfFrom.getPredecessors().size() == 1) { - MutableBasicBlock singlePreviousBlock = blockOfFrom.getPredecessors().get(0); - if (!singlePreviousBlock.getTail().branches() && singlePreviousBlock != blockOfFrom) { - if (singlePreviousBlock - .getExceptionalSuccessors() - .equals(blockOfFrom.getExceptionalSuccessors())) { - blockOfFrom - .getStmts() - .forEach( - k -> { - addNodeToBlock(blockOfFrom, k); - }); - return; - } - } + if (blockOfFrom.getTail() != from || blockOfTo.getHead() != to) { + // `from` and `to` aren't the tail and head of their respective blocks, + // which means they aren't connected + return false; } - // remove outgoing connections from blockA if from stmt is the tail - if (!from.branches()) { - if (!blockOfFrom.getStmts().isEmpty() && blockOfFrom.getSuccessors().size() == 1) { - // merge previous block if possible i.e. no branchingstmt as tail && same traps && no - // other predesccorblocks - MutableBasicBlock singleSuccessorBlock = blockOfFrom.getSuccessors().get(0); - if (singleSuccessorBlock.getPredecessors().size() == 1 - && singleSuccessorBlock.getPredecessors().get(0) == blockOfFrom) { - if (singleSuccessorBlock - .getExceptionalSuccessors() - .equals(blockOfFrom.getExceptionalSuccessors())) { - singleSuccessorBlock - .getStmts() - .forEach( - k -> { - addNodeToBlock(blockOfFrom, k); - }); - } - } - } - } else { - blockOfFrom.clearSuccessorBlocks(); + // remove the connection between the two blocks + boolean predecessorRemoved = blockOfTo.removePredecessorBlock(blockOfFrom); + boolean successorRemoved = blockOfFrom.removeSuccessorBlock(blockOfTo); + assert predecessorRemoved == successorRemoved; + + if (!predecessorRemoved) { + // the blocks weren't connected + return false; } + + // the removal of the edge between `from` and `to` might have created blocks that can be merged + tryMergeWithPredecessorBlock(blockOfTo); + tryMergeWithSuccessorBlock(blockOfFrom); + + return true; } } diff --git a/sootup.core/src/main/java/sootup/core/graph/MutableStmtGraph.java b/sootup.core/src/main/java/sootup/core/graph/MutableStmtGraph.java index 278b27a6143..72263177378 100644 --- a/sootup.core/src/main/java/sootup/core/graph/MutableStmtGraph.java +++ b/sootup.core/src/main/java/sootup/core/graph/MutableStmtGraph.java @@ -96,8 +96,12 @@ public void setEdges(@Nonnull Stmt from, @Nonnull Stmt... targets) { setEdges(from, Arrays.asList(targets)); } - /** removes the current outgoing flows of "from" to "targets" */ - public abstract void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to); + /** + * removes the current outgoing flows of "from" to "to" + * + * @return true if the edge existed and was removed; false if the edge didn't exist + */ + public abstract boolean removeEdge(@Nonnull Stmt from, @Nonnull Stmt to); /** Modifications of exceptional flows removes all exceptional flows from "stmt" */ public abstract void clearExceptionalEdges(@Nonnull Stmt stmt); diff --git a/sootup.core/src/test/java/sootup/core/graph/MutableBlockStmtGraphTest.java b/sootup.core/src/test/java/sootup/core/graph/MutableBlockStmtGraphTest.java index b74b27102cb..07f40952e80 100644 --- a/sootup.core/src/test/java/sootup/core/graph/MutableBlockStmtGraphTest.java +++ b/sootup.core/src/test/java/sootup/core/graph/MutableBlockStmtGraphTest.java @@ -256,11 +256,11 @@ public void modifyStmtToBlockAtTail() { assertEquals(1, graph.getBlocksSorted().get(1).getSuccessors().size()); // remove non-existing edge - graph.removeEdge(firstNop, conditionalStmt); + assertFalse(graph.removeEdge(firstNop, conditionalStmt)); assertEquals(2, graph.getBlocksSorted().size()); // remove branchingstmt at end -> edge across blocks - graph.removeEdge(conditionalStmt, firstNop); + assertTrue(graph.removeEdge(conditionalStmt, firstNop)); assertEquals(2, graph.getBlocksSorted().size()); assertEquals(4, graph.getNodes().size()); @@ -268,7 +268,7 @@ public void modifyStmtToBlockAtTail() { assertEquals(3, graph.getNodes().size()); // remove branchingstmt at head - graph.removeEdge(conditionalStmt, secondNop); + assertTrue(graph.removeEdge(conditionalStmt, secondNop)); assertEquals(1, graph.getBlocksSorted().size()); assertEquals(3, graph.getNodes().size()); @@ -402,22 +402,22 @@ public void linkDirectlyAddedBlocks() { assertEquals(1, graph.successors(firstNop).size()); assertEquals(1, graph.successors(secondNop).size()); - graph.removeEdge(secondNop, thirdNop); + assertTrue(graph.removeEdge(secondNop, thirdNop)); assertEquals(2, graph.getBlocksSorted().size()); assertEquals(1, graph.successors(firstNop).size()); assertEquals(0, graph.successors(secondNop).size()); - graph.removeEdge(secondNop, thirdNop); // empty operation + assertFalse(graph.removeEdge(secondNop, thirdNop)); // empty operation assertEquals(2, graph.getBlocksSorted().size()); assertEquals(1, graph.successors(firstNop).size()); assertEquals(0, graph.successors(secondNop).size()); - graph.removeEdge(firstNop, thirdNop); // empty operation + assertFalse(graph.removeEdge(firstNop, thirdNop)); // empty operation assertEquals(2, graph.getBlocksSorted().size()); assertEquals(1, graph.successors(firstNop).size()); assertEquals(0, graph.successors(secondNop).size()); - graph.removeEdge(firstNop, secondNop); + assertTrue(graph.removeEdge(firstNop, secondNop)); assertEquals(3, graph.getBlocksSorted().size()); } @@ -929,7 +929,7 @@ public void removeEdge() { assertEquals(1, graph.successors(stmt1).size()); assertTrue(graph.hasEdgeConnecting(stmt1, stmt2)); - graph.removeEdge(stmt1, stmt2); + assertTrue(graph.removeEdge(stmt1, stmt2)); assertEquals(0, graph.successors(stmt1).size()); assertFalse(graph.hasEdgeConnecting(stmt1, stmt2)); } @@ -960,7 +960,7 @@ public void removeImpossibleEdge() { Stmt stmt2 = new JNopStmt(StmtPositionInfo.createNoStmtPositionInfo()); MutableStmtGraph graph = new MutableBlockStmtGraph(); // nodes are not in the graph! - graph.removeEdge(stmt1, stmt2); + assertFalse(graph.removeEdge(stmt1, stmt2)); } @Test