Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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 has 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`.
*
* 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 has 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
// 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
// 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 = 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 has been deprecated in favor of the newly added `dominatingEdge`
predicate.
Loading
Loading