Skip to content
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

Shared: Use edge dominance terminology in basic block library #18696

Merged
merged 6 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from 4 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
6 changes: 6 additions & 0 deletions csharp/ql/lib/change-notes/2025-02-11-basic-block-rename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: deprecated
---
* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
class have been deprecated in favor of the newly added `dominatingEdge`
predicate.
34 changes: 30 additions & 4 deletions csharp/ql/lib/semmle/code/csharp/controlflow/BasicBlocks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,29 @@ final class BasicBlock extends BasicBlocksImpl::BasicBlock {
*/
BasicBlock getImmediateDominator() { result = super.getImmediateDominator() }

/**
* Holds if the edge with successor type `s` out of this basic block is a
* dominating edge for `dominated`.
*
* That is, all paths reaching `dominated` from the entry point basic
* block must go through the `s` edge out of this basic block.
*
* Edge dominance is similar to node dominance except it concerns edges
* instead of nodes: A basic block is dominated by a _basic block_ `bb` if it
* can only be reached through `bb` and dominated by an _edge_ `s` if it can
* only be reached through `s`.
paldepind marked this conversation as resolved.
Show resolved Hide resolved
*
* Note that where all basic blocks (except the entry basic block) are
* strictly dominated by at least one basic block, a basic block may not be
* dominated by any edge. If an edge dominates a basic block `bb`, then
* both endpoints of the edge dominates `bb`. The converse is not the case,
* as there may be multiple paths between the endpoints with none of them
* dominating.
*/
predicate edgeDominates(BasicBlock dominated, ControlFlow::SuccessorType s) {
super.edgeDominates(dominated, s)
}

/**
* Holds if this basic block strictly post-dominates basic block `bb`.
*
Expand Down Expand Up @@ -296,11 +319,14 @@ final class JoinBlockPredecessor extends BasicBlock, BasicBlocksImpl::JoinPredec
* control flow.
*/
final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBlock {
predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
super.immediatelyControls(succ, s)
/** DEPRECATED: Use `edgeDominates` instead. */
deprecated predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
this.getASuccessor(s) = succ and
BasicBlocksImpl::dominatingEdge(this, succ)
}

predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
super.controls(controlled, s)
/** DEPRECATED: Use `edgeDominates` instead. */
deprecated predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
super.edgeDominates(controlled, s)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,6 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
this.controlsBlockSplit(controlled, s, cb)
or
cb.getLastNode() = this.getAControlFlowNode() and
cb.controls(controlled, s)
cb.edgeDominates(controlled, s)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s |
guard.getAControlFlowNode() = conditionBlock.getLastNode() and
s.getValue() = branch and
conditionBlock.controls(bb, s)
conditionBlock.edgeDominates(bb, s)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import ControlFlow
query predicate conditionBlock(
BasicBlocks::ConditionBlock cb, BasicBlock controlled, boolean testIsTrue
) {
cb.controls(controlled, any(SuccessorTypes::ConditionalSuccessor s | testIsTrue = s.getValue()))
cb.edgeDominates(controlled,
any(SuccessorTypes::ConditionalSuccessor s | testIsTrue = s.getValue()))
}

ControlFlow::Node successor(ControlFlow::Node node, boolean kind) {
Expand Down
6 changes: 6 additions & 0 deletions ruby/ql/lib/change-notes/2025-02-11-basic-block-rename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: deprecated
---
* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
class have been deprecated in favor of the newly added `dominatingEdge`
predicate.
36 changes: 32 additions & 4 deletions ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,29 @@ final class BasicBlock extends BasicBlocksImpl::BasicBlock {
*/
BasicBlock getImmediateDominator() { result = super.getImmediateDominator() }

/**
* Holds if the edge with successor type `s` out of this basic block is a
* dominating edge for `dominated`.
*
* That is, all paths reaching `dominated` from the entry point basic
* block must go through the `s` edge out of this basic block.
*
* Edge dominance is similar to node dominance except it concerns edges
* instead of nodes: A basic block is dominated by a _basic block_ `bb` if it
* can only be reached through `bb` and dominated by an _edge_ `s` if it can
* only be reached through `s`.
Copy link
Contributor

Choose a reason for hiding this comment

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

same

*
* Note that where all basic blocks (except the entry basic block) are
* strictly dominated by at least one basic block, a basic block may not be
* dominated by any edge. If an edge dominates a basic block `bb`, then
* both endpoints of the edge dominates `bb`. The converse is not the case,
* as there may be multiple paths between the endpoints with none of them
* dominating.
*/
predicate edgeDominates(BasicBlock dominated, SuccessorType s) {
super.edgeDominates(dominated, s)
}

/**
* Holds if this basic block strictly post-dominates basic block `bb`.
*
Expand Down Expand Up @@ -248,21 +271,26 @@ final class JoinBlockPredecessor extends BasicBlock, BasicBlocksImpl::JoinPredec
*/
final class ConditionBlock extends BasicBlock, BasicBlocksImpl::ConditionBasicBlock {
/**
* DEPRECATED: Use `edgeDominates` instead.
*
* Holds if basic block `succ` is immediately controlled by this basic
* block with conditional value `s`. That is, `succ` is an immediate
* successor of this block, and `succ` can only be reached from
* the callable entry point by going via the `s` edge out of this basic block.
*/
predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
super.immediatelyControls(succ, s)
deprecated predicate immediatelyControls(BasicBlock succ, ConditionalSuccessor s) {
this.getASuccessor(s) = succ and
BasicBlocksImpl::dominatingEdge(this, succ)
}

/**
* DEPRECATED: Use `edgeDominates` instead.
*
* Holds if basic block `controlled` is controlled by this basic block with
* conditional value `s`. That is, `controlled` can only be reached from the
* callable entry point by going via the `s` edge out of this basic block.
*/
predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
super.controls(controlled, s)
deprecated predicate controls(BasicBlock controlled, ConditionalSuccessor s) {
super.edgeDominates(controlled, s)
}
}
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean
exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
guard = conditionBlock.getLastNode() and
s.getValue() = branch and
conditionBlock.controls(bb, s)
conditionBlock.edgeDominates(bb, s)
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -2387,7 +2387,7 @@ module TypeInference {
|
m = resolveConstantReadAccess(pattern.getExpr()) and
cb.getLastNode() = pattern and
cb.controls(read.getBasicBlock(),
cb.edgeDominates(read.getBasicBlock(),
any(SuccessorTypes::MatchingSuccessor match | match.getValue() = true)) and
caseRead = def.getARead() and
read = def.getARead() and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ module ConditionalBypass {

SensitiveActionGuardConditional() {
exists(ConditionBlock cb, BasicBlock controlled |
cb.controls(controlled, _) and
cb.edgeDominates(controlled, _) and
controlled.getANode() = action.asExpr() and
cb.getLastNode() = this.asExpr()
)
Expand Down
4 changes: 2 additions & 2 deletions ruby/ql/test/library-tests/controlflow/graph/BasicBlocks.ql
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ query predicate immediateDominator(BasicBlock bb1, BasicBlock bb2) {
bb1.getImmediateDominator() = bb2
}

query predicate controls(ConditionBlock bb1, BasicBlock bb2, SuccessorType t) {
bb1.controls(bb2, t)
query predicate controls(ConditionBlock bb1, BasicBlock bb2, SuccessorTypes::ConditionalSuccessor t) {
bb1.edgeDominates(bb2, t)
}

query predicate successor(ConditionBlock bb1, BasicBlock bb2, SuccessorType t) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ query predicate newStyleBarrierGuards(DataFlow::Node n) {

query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::ConditionalSuccessor s) {
exists(ConditionBlock cb |
cb.controls(bb, s) and
cb.edgeDominates(bb, s) and
condition = cb.getLastNode()
)
}
Expand Down
2 changes: 1 addition & 1 deletion rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
exists(ConditionBasicBlock conditionBlock, ConditionalSuccessor s |
guard = conditionBlock.getLastNode() and
s.getValue() = branch and
conditionBlock.controls(bb, s)
conditionBlock.edgeDominates(bb, s)
)
}

Expand Down
2 changes: 1 addition & 1 deletion rust/ql/test/library-tests/controlflow/BasicBlocks.ql
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ query predicate immediateDominator(BasicBlock bb1, BasicBlock bb2) {
}

query predicate controls(ConditionBasicBlock bb1, BasicBlock bb2, SuccessorType t) {
bb1.controls(bb2, t)
bb1.edgeDominates(bb2, t)
}

query predicate successor(ConditionBasicBlock bb1, BasicBlock bb2, SuccessorType t) {
Expand Down
103 changes: 52 additions & 51 deletions shared/controlflow/codeql/controlflow/BasicBlock.qll
Original file line number Diff line number Diff line change
Expand Up @@ -169,90 +169,59 @@ module Make<LocationSig Location, InputSig<Location> Input> {
BasicBlock getImmediateDominator() { bbIDominates(result, this) }

/**
* Holds if basic block `succ` is immediately controlled by this basic
* block with successor type `s`.
* Holds if the edge with successor type `s` out of this basic block is a
* dominating edge for `dominated`.
*
* That is, `succ` is an immediate successor of this block, and `succ` can
* only be reached from the entry block by going via the `s` edge out of
* this basic block.
*/
pragma[nomagic]
predicate immediatelyControls(BasicBlock succ, SuccessorType s) {
succ = this.getASuccessor(s) and
bbIDominates(this, succ) and
// The above is not sufficient to ensure that `succ` can only be reached
// through `s`. To see why, consider this example corresponding to an
// `if` statement without an `else` block and whe `A` is the basic block
// following the `if` statement:
// ```
// ... --> cond --[true]--> ... --> A
// \ /
// ----[false]-----------
// ```
// Here `A` is a direct successor of `cond` along the `false` edge and it
// is immediately dominated by `cond`, but `A` is not controlled by the
// `false` edge since it is also possible to reach `A` via the `true`
// edge.
//
// Note that the first and third conjunct implies the second. But
// explicitly including the second conjunct leads to a better join order.
forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != this |
succ.dominates(pred)
)
}

/**
* Holds if basic block `controlled` is controlled by this basic block with
* successor type `s`.
*
* That is, all paths reaching `controlled` from the entry point basic
* That is, all paths reaching `dominated` from the entry point basic
* block must go through the `s` edge out of this basic block.
*
* Control is similar to dominance except it concerns edges instead of
* nodes: A basic block is _dominated_ by a _basic block_ `bb` if it can
* only be reached through `bb` and _controlled_ by an _edge_ `s` if it can
* only be reached through `s`.
* Edge dominance is similar to node dominance except it concerns edges
* instead of nodes: A basic block is dominated by a _basic block_ `bb` if
* it can only be reached through `bb` and dominated by an _edge_ `s` if it
* can only be reached through `s`.
Copy link
Contributor

Choose a reason for hiding this comment

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

same

*
* Note that where all basic blocks (except the entry basic block) are
* strictly dominated by at least one basic block, a basic block may not be
* controlled by any edge. If an edge controls a basic block `bb`, then
* dominated by any edge. If an edge dominates a basic block `bb`, then
* both endpoints of the edge dominates `bb`. The converse is not the case,
* as there may be multiple paths between the endpoints with none of them
* dominating.
*/
predicate controls(BasicBlock controlled, SuccessorType s) {
// For this block to control the block `controlled` with `s` the following must be true:
// 1/ Execution must have passed through the test i.e. `this` must strictly dominate `controlled`.
predicate edgeDominates(BasicBlock dominated, SuccessorType s) {
// For this block to control the block `dominated` with `s` the following must be true:
// 1/ Execution must have passed through the test i.e. `this` must strictly dominate `dominated`.
// 2/ Execution must have passed through the `s` edge leaving `this`.
//
// Although "passed through the `s` edge" implies that `this.getASuccessor(s)` dominates `controlled`,
// Although "passed through the `s` edge" implies that `this.getASuccessor(s)` dominates `dominated`,
// the reverse is not true, as flow may have passed through another edge to get to `this.getASuccessor(s)`
// so we need to assert that `this.getASuccessor(s)` dominates `controlled` *and* that
// so we need to assert that `this.getASuccessor(s)` dominates `dominated` *and* that
// all predecessors of `this.getASuccessor(s)` are either `this` or dominated by `this.getASuccessor(s)`.
//
// For example, in the following C# snippet:
// ```csharp
// if (x)
// controlled;
// dominated;
// false_successor;
// uncontrolled;
// ```
// `false_successor` dominates `uncontrolled`, but not all of its predecessors are `this` (`if (x)`)
// or dominated by itself. Whereas in the following code:
// ```csharp
// if (x)
// while (controlled)
// while (dominated)
// also_controlled;
// false_successor;
// uncontrolled;
// ```
// the block `while controlled` is controlled because all of its predecessors are `this` (`if (x)`)
// the block `while dominated` is dominated because all of its predecessors are `this` (`if (x)`)
// or (in the case of `also_controlled`) dominated by itself.
//
// The additional constraint on the predecessors of the test successor implies
// that `this` strictly dominates `controlled` so that isn't necessary to check
// that `this` strictly dominates `dominated` so that isn't necessary to check
// directly.
exists(BasicBlock succ | this.immediatelyControls(succ, s) | succ.dominates(controlled))
exists(BasicBlock succ |
succ = this.getASuccessor(s) and dominatingEdge(this, succ) and succ.dominates(dominated)
)
}

/**
Expand Down Expand Up @@ -282,6 +251,38 @@ module Make<LocationSig Location, InputSig<Location> Input> {
string toString() { result = this.getFirstNode().toString() }
}

/**
* Holds if `bb1` has `bb2` as a direct successor and the edge between `bb1`
* and `bb2` is a dominating edge.
*
* An edge `(bb1, bb2)` is dominating if there exists a basic block that can
* only be reached from the entry block by going through `(bb1, bb2)`. This
* implies that `(bb1, bb2)` dominates its endpoint `bb2`. I.e., `bb2` can
* only be reached from the entry block by going via `(bb1, bb2)`.
*/
pragma[nomagic]
predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2) {
bb1.getASuccessor(_) = bb2 and
bbIDominates(bb1, bb2) and
// The above is not sufficient to ensure that `bb1` can only be reached
paldepind marked this conversation as resolved.
Show resolved Hide resolved
// through `(bb1, bb2)`. To see why, consider this example corresponding to
// an `if` statement without an `else` block and whe `A` is the basic block
paldepind marked this conversation as resolved.
Show resolved Hide resolved
// following the `if` statement:
// ```
// ... --> cond --[true]--> ... --> A
// \ /
paldepind marked this conversation as resolved.
Show resolved Hide resolved
// ----[false]-----------
// ```
// Here `A` is a direct successor of `cond` along the `false` edge and it
// is immediately dominated by `cond`, but `A` is not controlled by the
paldepind marked this conversation as resolved.
Show resolved Hide resolved
// `false` edge since it is also possible to reach `A` via the `true`
// edge.
//
// Note that the first and third conjunct implies the second. But
// explicitly including the second conjunct leads to a better join order.
forall(BasicBlock pred | pred = bb2.getAPredecessor() and pred != bb1 | bb2.dominates(pred))
}

cached
private module Cached {
/**
Expand Down
2 changes: 2 additions & 0 deletions shared/controlflow/codeql/controlflow/Cfg.qll
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,8 @@ module MakeWithSplitting<

final class BasicBlock = BasicBlockImpl::BasicBlock;

predicate dominatingEdge = BasicBlockImpl::dominatingEdge/2;

/**
* An entry basic block, that is, a basic block whose first node is
* an entry node.
Expand Down
6 changes: 6 additions & 0 deletions swift/ql/lib/change-notes/2025-02-11-basic-block-rename.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: deprecated
---
* The predicates `immediatelyControls` and `controls` on the `ConditionBlock`
class have been deprecated in favor of the newly added `dominatingEdge`
predicate.
Loading
Loading