Skip to content

Swift: switch to shared, parameterized CFG library #15219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion swift/ql/consistency-queries/CfgConsistency.ql
Original file line number Diff line number Diff line change
@@ -1 +1 @@
import codeql.swift.controlflow.internal.ControlFlowGraphImplShared::Consistency
import codeql.swift.controlflow.internal.ControlFlowGraphImplSpecific::CfgImpl::Consistency
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The control flow graph library (`codeql.swift.controlflow`) has been transitioned to use the shared implementation from the `codeql/controlflow` qlpack. No result changes are expected due to this change.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I would have put a change note here (since no users are expected to notice this change), but I guess it doesn't hurt 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's my fault - I asked for a change note because the check was failing, without thinking about whether should have just added the flag to say that one is not required. I agree it doesn't do any harm.

12 changes: 8 additions & 4 deletions swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

private import swift
private import ControlFlowGraph
private import internal.ControlFlowGraphImpl
private import internal.ControlFlowGraphImpl as Impl
private import internal.ControlFlowElements
private import CfgNodes
private import SuccessorTypes
Expand Down Expand Up @@ -133,7 +133,9 @@ private module Cached {
private predicate predBB(BasicBlock succ, BasicBlock pred) { succBB(pred, succ) }

/** Holds if `bb` is an exit basic block that represents normal exit. */
private predicate normalExitBB(BasicBlock bb) { bb.getANode().(AnnotatedExitNode).isNormal() }
private predicate normalExitBB(BasicBlock bb) {
bb.getANode().(Impl::AnnotatedExitNode).isNormal()
}

/** Holds if `dom` is an immediate post-dominator of `bb`. */
cached
Expand Down Expand Up @@ -167,7 +169,9 @@ private predicate entryBB(BasicBlock bb) { bb.getFirstNode() instanceof EntryNod
class EntryBasicBlock extends BasicBlock {
EntryBasicBlock() { entryBB(this) }

override CfgScope getScope() { this.getFirstNode() = TEntryNode(result) }
override CfgScope getScope() {
this.getFirstNode() = any(EntryNode node | node.getScope() = result)
}
}

/**
Expand All @@ -178,7 +182,7 @@ class AnnotatedExitBasicBlock extends BasicBlock {
private boolean normal;

AnnotatedExitBasicBlock() {
exists(AnnotatedExitNode n |
exists(Impl::AnnotatedExitNode n |
n = this.getANode() and
if n.isNormal() then normal = true else normal = false
)
Expand Down
111 changes: 27 additions & 84 deletions swift/ql/lib/codeql/swift/controlflow/CfgNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -3,108 +3,39 @@
private import swift
private import BasicBlocks
private import ControlFlowGraph
private import internal.ControlFlowGraphImpl
private import internal.ControlFlowGraphImpl as Impl
private import internal.ControlFlowElements
private import internal.Splitting

/** An entry node for a given scope. */
class EntryNode extends ControlFlowNode, TEntryNode {
private CfgScope scope;

EntryNode() { this = TEntryNode(scope) }

final override EntryBasicBlock getBasicBlock() { result = ControlFlowNode.super.getBasicBlock() }

final override Location getLocation() { result = scope.getLocation() }

final override string toString() { result = "enter " + scope }
}

/** An exit node for a given scope, annotated with the type of exit. */
class AnnotatedExitNode extends ControlFlowNode, TAnnotatedExitNode {
private CfgScope scope;
private boolean normal;

AnnotatedExitNode() { this = TAnnotatedExitNode(scope, normal) }

/** Holds if this node represent a normal exit. */
final predicate isNormal() { normal = true }

final override AnnotatedExitBasicBlock getBasicBlock() {
result = ControlFlowNode.super.getBasicBlock()
}

final override Location getLocation() { result = scope.getLocation() }

final override string toString() {
exists(string s |
normal = true and s = "normal"
or
normal = false and s = "abnormal"
|
result = "exit " + scope + " (" + s + ")"
)
}
}

/** An exit node for a given scope. */
class ExitNode extends ControlFlowNode, TExitNode {
private CfgScope scope;

ExitNode() { this = TExitNode(scope) }

final override Location getLocation() { result = scope.getLocation() }

final override string toString() { result = "exit " + scope }
}

/**
* A node for an AST node.
*
* Each AST node maps to zero or more `CfgNode`s: zero when the node is unreachable
* (dead) code or not important for control flow, and multiple when there are different
* splits for the AST node.
*/
class CfgNode extends ControlFlowNode, TElementNode {
private Splits splits;
ControlFlowElement n;

CfgNode() { this = TElementNode(_, n, splits) }

final override ControlFlowElement getNode() { result = n }
class CfgNode extends ControlFlowNode instanceof Impl::AstCfgNode {
final override ControlFlowElement getNode() { result = this.getAstNode() }

override Location getLocation() { result = n.getLocation() }

final override string toString() {
exists(string s | s = n.toString() |
result = "[" + this.getSplitsString() + "] " + s
or
not exists(this.getSplitsString()) and result = s
)
}
/** Gets a split for this control flow node, if any. */
final Split getASplit() { result = super.getASplit() }

/** Gets a comma-separated list of strings for each split in this node, if any. */
final string getSplitsString() {
result = splits.toString() and
result != ""
}

/** Gets a split for this control flow node, if any. */
final Split getASplit() { result = splits.getASplit() }
final string getSplitsString() { result = super.getSplitsString() }

/** Gets the AST representation of this control flow node, if any. */
Expr getAst() {
result = n.asAstNode()
result = this.getNode().asAstNode()
or
result = n.(PropertyGetterElement).getRef()
result = this.getNode().(PropertyGetterElement).getRef()
or
result = n.(PropertySetterElement).getAssignExpr()
result = this.getNode().(PropertySetterElement).getAssignExpr()
or
result = n.(PropertyObserverElement).getAssignExpr()
result = this.getNode().(PropertyObserverElement).getAssignExpr()
or
result = n.(ClosureElement).getAst()
result = this.getNode().(ClosureElement).getAst()
or
result = n.(KeyPathElement).getAst()
result = this.getNode().(KeyPathElement).getAst()
}
}

Expand All @@ -130,7 +61,9 @@ class PatternCfgNode extends CfgNode {

/** A control-flow node that wraps a property getter. */
class PropertyGetterCfgNode extends CfgNode {
override PropertyGetterElement n;
PropertyGetterElement n;

PropertyGetterCfgNode() { n = this.getAstNode() }

Expr getRef() { result = n.getRef() }

Expand All @@ -141,7 +74,9 @@ class PropertyGetterCfgNode extends CfgNode {

/** A control-flow node that wraps a property setter. */
class PropertySetterCfgNode extends CfgNode {
override PropertySetterElement n;
PropertySetterElement n;

PropertySetterCfgNode() { n = this.getAstNode() }

AssignExpr getAssignExpr() { result = n.getAssignExpr() }

Expand All @@ -153,7 +88,9 @@ class PropertySetterCfgNode extends CfgNode {
}

class PropertyObserverCfgNode extends CfgNode {
override PropertyObserverElement n;
PropertyObserverElement n;

PropertyObserverCfgNode() { n = this.getAstNode() }

AssignExpr getAssignExpr() { result = n.getAssignExpr() }

Expand Down Expand Up @@ -201,3 +138,9 @@ class KeyPathApplicationExprCfgNode extends ExprCfgNode {
class KeyPathExprCfgNode extends ExprCfgNode {
override KeyPathExpr e;
}

class EntryNode = Impl::EntryNode;

class ExitNode = Impl::ExitNode;

class AnnotatedExitNode = Impl::AnnotatedExitNode;
10 changes: 2 additions & 8 deletions swift/ql/lib/codeql/swift/controlflow/ControlFlowGraph.qll
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,10 @@ class CfgScope extends Scope instanceof CfgScope::Range_ {
*
* Only nodes that can be reached from an entry point are included in the CFG.
*/
class ControlFlowNode extends TCfgNode {
/** Gets a textual representation of this control flow node. */
string toString() { none() }

class ControlFlowNode extends Node {
/** Gets the AST node that this node corresponds to, if any. */
ControlFlowElement getNode() { none() }

/** Gets the location of this control flow node. */
Location getLocation() { none() }

/** Gets the file of this control flow node. */
final File getFile() { result = this.getLocation().getFile() }

Expand All @@ -50,7 +44,7 @@ class ControlFlowNode extends TCfgNode {
BasicBlock getBasicBlock() { result.getANode() = this }

/** Gets a successor node of a given type, if any. */
final ControlFlowNode getASuccessor(SuccessorType t) { result = getASuccessor(this, t) }
final ControlFlowNode getASuccessor(SuccessorType t) { result = super.getASuccessor(t) }

/** Gets an immediate successor, if any. */
final ControlFlowNode getASuccessor() { result = this.getASuccessor(_) }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
private import swift
private import Completion
private import ControlFlowGraphImplShared
private import ControlFlowGraphImplSpecific::CfgImpl
private import ControlFlowElements

abstract class AstControlFlowTree extends ControlFlowTree {
Expand Down
Loading