From edd506864cd6a8351e8b655657415075a6c7bfdb Mon Sep 17 00:00:00 2001 From: KuechA <31155350+KuechA@users.noreply.github.com> Date: Thu, 6 Feb 2025 15:54:14 +0100 Subject: [PATCH] Simplified and more basic fixpoint iteration for `ControlDependenceGraphPass` (#1812) * Try a new eog iteration * Small changes * remove comment * small cleanup * CDG pass update * Cleanup * Cleanup imports * Test PowersetLattice * MapLattice test * Comment * Test tuple lattice * Test triple lattice * More equals * Update MapLattice iteration * More generics * Formatting * Make things compile again * Collection comprehensions also have some conditions * Fix bug in comparison * Fix bug in comparison * Remove debug things * Formatting * Try more efficient lub * Try more efficient lub and worklist * Fix cpg core * Reduce heap size again * Rename stuff * Explain and use the typealiases * Update description * Try more redesign * provide bottom element * Try to test * Some fixes, transfer tests * fix broken equals check * More redesign * Some more documentation * More open, minor fix * Fix bug from merge * Fix bug with element set * Update CDG pass accordingly * Remove obsolete files * Remove useless import * Improved performance options in identity set * Improved performance of comparisons and lub * Reduce amount of duplicate code * Adapt test to less copying of objects * IdentitySet coverage * Lattice compare coverage * More coverage of PowersetLattice * powerset lattice glb * MapLattice glb * Test coverage * Add TODO --- .../cpg/passes/ControlDependenceGraphPass.kt | 151 +++++++++--------- 1 file changed, 75 insertions(+), 76 deletions(-) diff --git a/cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPass.kt b/cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPass.kt index a73830f69be..cd9709d7e30 100644 --- a/cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPass.kt +++ b/cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPass.kt @@ -31,15 +31,20 @@ import de.fraunhofer.aisec.cpg.graph.Node import de.fraunhofer.aisec.cpg.graph.allChildren import de.fraunhofer.aisec.cpg.graph.declarations.FunctionDeclaration import de.fraunhofer.aisec.cpg.graph.declarations.cyclomaticComplexity -import de.fraunhofer.aisec.cpg.graph.edges.Edge import de.fraunhofer.aisec.cpg.graph.edges.flows.EvaluationOrder import de.fraunhofer.aisec.cpg.graph.statements.IfStatement import de.fraunhofer.aisec.cpg.graph.statements.ReturnStatement +import de.fraunhofer.aisec.cpg.graph.statements.expressions.CollectionComprehension +import de.fraunhofer.aisec.cpg.graph.statements.expressions.ComprehensionExpression import de.fraunhofer.aisec.cpg.graph.statements.expressions.ConditionalExpression import de.fraunhofer.aisec.cpg.graph.statements.expressions.ShortCircuitOperator -import de.fraunhofer.aisec.cpg.helpers.* +import de.fraunhofer.aisec.cpg.helpers.functional.Lattice +import de.fraunhofer.aisec.cpg.helpers.functional.MapLattice +import de.fraunhofer.aisec.cpg.helpers.functional.PowersetLattice +import de.fraunhofer.aisec.cpg.helpers.toIdentitySet import de.fraunhofer.aisec.cpg.passes.configuration.DependsOn -import java.util.* +import kotlin.collections.component1 +import kotlin.collections.component2 /** This pass builds the Control Dependence Graph (CDG) by iterating through the EOG. */ @DependsOn(EvaluationOrderGraphPass::class) @@ -84,26 +89,32 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( return } + val prevEOGState = + PrevEOGState(innerLattice = PrevEOGLattice(innerLattice = PowersetLattice())) + // Maps nodes to their "cdg parent" (i.e. the dominator) and also has the information // through which path it is reached. If all outgoing paths of the node's dominator result in // the node, we use the dominator's state instead (i.e., we move the node one layer upwards) - val startState = PrevEOGState() - val identityMap = IdentityHashMap>() - identityMap[startNode] = identitySetOf(startNode) - startState.push(startNode, PrevEOGLattice(identityMap)) - val finalState = iterateEOG(startNode.nextEOGEdges, startState, ::handleEdge) ?: return + var startState: PrevEOGStateElement = prevEOGState.bottom + startState = + prevEOGState.push( + startState, + startNode, + PrevEOGLatticeElement(startNode to PowersetLattice.Element(startNode)), + ) + log.debug("Iterating EOG of {}", startNode) + val finalState = prevEOGState.iterateEOG(startNode.nextEOGEdges, startState, ::handleEdge) + log.debug("Done iterating EOG of {}", startNode) val branchingNodeConditionals = getBranchingNodeConditions(startNode) // Collect the information, identify merge points, etc. This is not really efficient yet :( for ((node, dominatorPaths) in finalState) { val dominatorsList = - dominatorPaths.elements.entries - .map { (k, v) -> Pair(k, v.toMutableSet()) } - .toMutableList() + dominatorPaths.entries.map { (k, v) -> Pair(k, v.toMutableSet()) }.toMutableList() val finalDominators = mutableListOf>>() val conditionKeys = - dominatorPaths.elements.entries + dominatorPaths.entries .filter { (k, _) -> (k as? BranchingNode)?.branchedBy == node || node in @@ -116,7 +127,7 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( // want this. Move it one layer up. for (k1 in conditionKeys) { dominatorsList.removeIf { k1 == it.first } - finalState[k1]?.elements?.forEach { (newK, newV) -> + finalState[k1]?.forEach { (newK, newV) -> val entry = dominatorsList.firstOrNull { it.first == newK } entry?.let { dominatorsList.remove(entry) @@ -135,7 +146,7 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( // We are reachable from all the branches of a branching node. Add this parent // to the worklist or update an existing entry. Also consider already existing // entries in finalDominators list and update it (if necessary) - val newDominatorMap = finalState[k]?.elements + val newDominatorMap = finalState[k] newDominatorMap?.forEach { (newK, newV) -> when { dominatorsList.any { it.first == newK } -> { @@ -162,14 +173,14 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( it.first == newK && it.second.containsAll(newV) } -> { // We don't have an entry yet => add a new one - val newEntry = Pair(newK, newV.toMutableSet()) + val newEntry = Pair(newK, newV.toIdentitySet()) dominatorsList.add(newEntry) } else -> { // Not sure what to do, there seems to be a cycle but this entry is // not in finalDominators for some reason. Add to finalDominators // now. - finalDominators.add(Pair(newK, newV.toMutableSet())) + finalDominators.add(Pair(newK, newV.toIdentitySet())) } } } @@ -225,8 +236,11 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( // For the function declaration, there's only the path through the function declaration // itself. Pair(functionDeclaration, setOf(functionDeclaration)), - *functionDeclaration - .allChildren() + *(functionDeclaration.allChildren() + + functionDeclaration.allChildren() + + functionDeclaration.allChildren< + ComprehensionExpression + >()) // TODO: May be simplified when resolving issue 2027 .filterIsInstance() .map { branchingNode -> val mergingPoints = @@ -262,27 +276,37 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass( * Returns the updated state and true because we always expect an update of the state. */ fun handleEdge( - currentEdge: Edge, - currentState: State>>, -): State>> { + lattice: Lattice, + currentEdge: EvaluationOrder, + currentState: PrevEOGStateElement, +): PrevEOGStateElement { + val lattice = lattice as? PrevEOGState ?: return currentState + var newState = currentState + + val currentStart = currentEdge.start + // Check if we start in a branching node and if this edge leads to the conditional // branch. In this case, the next node will move "one layer downwards" in the CDG. - if (currentEdge.start is BranchingNode) { // && currentEdge.isConditionalBranch()) { + if ( + currentStart is BranchingNode || + currentStart is ComprehensionExpression || + currentStart.astParent is + ComprehensionExpression && // TODO: May be simplified when resolving issue 2027 + currentStart == (currentStart.astParent as ComprehensionExpression).iterable + ) { // && currentEdge.isConditionalBranch()) { // We start in a branching node and end in one of the branches, so we have the // following state: // for the branching node "start", we have a path through "end". val prevPathLattice = - PrevEOGLattice( - IdentityHashMap( - currentState[currentEdge.start]?.elements?.filter { (k, _) -> - k == currentEdge.start - } - ) + PrevEOGLatticeElement( + newState[currentStart] + ?.filter { (k, _) -> k == currentStart } + ?.mapValues { (_, v) -> PowersetLattice.Element(v) } ?: mapOf() ) - val map = IdentityHashMap>() - map[currentEdge.start] = identitySetOf(currentEdge.end) - val newPath = PrevEOGLattice(map).lub(prevPathLattice) as PrevEOGLattice - currentState.push(currentEdge.end, newPath) + val map = PrevEOGLatticeElement(currentStart to PowersetLattice.Element(currentEdge.end)) + + val newPath = lattice.innerLattice.lub(PrevEOGLatticeElement(map), prevPathLattice) + newState = lattice.push(newState, currentEdge.end, newPath) } else { // We did not start in a branching node, so for the next node, we have the same path // (last branching + first end node) as for the start node of this edge. @@ -290,15 +314,13 @@ fun handleEdge( // first edge in a function), we generate a new state where we start in "start" end // have "end" as the first node in the "branch". val state = - PrevEOGLattice( - currentState[currentEdge.start]?.elements - ?: IdentityHashMap( - mutableMapOf(Pair(currentEdge.start, identitySetOf(currentEdge.end))) - ) + PrevEOGLatticeElement( + newState[currentStart]?.mapValues { (_, v) -> PowersetLattice.Element(v) } + ?: mutableMapOf(Pair(currentStart, PowersetLattice.Element(currentEdge.end))) ) - currentState.push(currentEdge.end, state) + newState = lattice.push(newState, currentEdge.end, state) } - return currentState + return newState } /** @@ -319,6 +341,9 @@ private fun EvaluationOrder.isConditionalBranch(): Boolean { true } else (this.start is IfStatement || + this.start is ComprehensionExpression || + (this.start.astParent is ComprehensionExpression && + this.start == (this.start.astParent as ComprehensionExpression).iterable) || this.start is ConditionalExpression || this.start is ShortCircuitOperator) && branch == false || (this.start is IfStatement && @@ -353,44 +378,18 @@ private fun IfStatement.allBranchesFromMyThenBranchGoThrough(node: Node?): Boole return true } -/** - * Implements the [LatticeElement] over a set of nodes and their set of "nextEOG" nodes which reach - * this node. - */ -class PrevEOGLattice(override val elements: IdentityHashMap>) : - LatticeElement>>(elements) { +typealias PrevEOGLatticeElement = MapLattice.Element> - override fun lub( - other: LatticeElement>> - ): LatticeElement>> { - val newMap = IdentityHashMap(other.elements.mapValues { (_, v) -> v.toIdentitySet() }) - for ((key, value) in this.elements) { - newMap.computeIfAbsent(key, ::identitySetOf).addAll(value) - } - return PrevEOGLattice(newMap) - } +typealias PrevEOGLattice = MapLattice> - override fun duplicate() = PrevEOGLattice(IdentityHashMap(this.elements)) +typealias PrevEOGStateElement = MapLattice.Element - override fun compareTo(other: LatticeElement>>): Int { - return if ( - this.elements.keys.containsAll(other.elements.keys) && - this.elements.all { (k, v) -> v.containsAll(other.elements[k] ?: identitySetOf()) } - ) { - if ( - this.elements.keys.size > (other.elements.keys.size) || - this.elements.any { (k, v) -> v.size > (other.elements[k]?.size ?: 0) } - ) - 1 - else 0 - } else { - -1 - } - } -} +typealias PrevEOGState = MapLattice -/** - * A state which actually holds a state for all [Edge]s. It maps the node to its - * [BranchingNode]-parent and the path through which it is reached. - */ -class PrevEOGState : State>>() +fun PrevEOGState.push( + currentElement: PrevEOGStateElement, + newNode: Node, + newEOGLattice: PrevEOGLatticeElement, +): PrevEOGStateElement { + return this.lub(currentElement, PrevEOGStateElement(newNode to newEOGLattice)) +}