Skip to content

SSA: Replace the Guards interface in the SSA data flow integration. #18934

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.

Sign up for GitHub

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
aschackmull merged 3 commits into from
Mar 6, 2025
Merged

SSA: Replace the Guards interface in the SSA data flow integration. #18934

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a027353
Ruby: Remove some DefinitionExt references and deprecate the rest.
aschackmull Feb 27, 2025
c6761db
SSA: Replace the Guards interface in the SSA data flow integration.
aschackmull Mar 5, 2025
5e722ee
Ruby: Push in casts to Definition to delete the then unused Definitio…
aschackmull Mar 6, 2025
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
23 changes: 11 additions & 12 deletions csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,8 +1047,17 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
}

class Guard extends Guards::Guard {
predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) {
this.getAControlFlowNode() = bb.getNode(i)
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) {
exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s |
this.getAControlFlowNode() = bb1.getLastNode() and
bb2 = bb1.getASuccessorByType(s) and
s.getValue() = branch
)
}
}

Expand All @@ -1060,16 +1069,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
conditionBlock.edgeDominates(bb, s)
)
}

/** Gets an immediate conditional successor of basic block `bb`, if any. */
ControlFlow::BasicBlock getAConditionalBasicBlockSuccessor(
ControlFlow::BasicBlock bb, boolean branch
) {
exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s |
result = bb.getASuccessorByType(s) and
s.getValue() = branch
)
}
}

private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
16 changes: 7 additions & 9 deletions java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,22 +667,20 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
}

class Guard extends Guards::Guard {
predicate hasCfgNode(BasicBlock bb, int i) {
this = bb.getNode(i).asExpr()
or
this = bb.getNode(i).asStmt()
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) {
super.hasBranchEdge(bb1, bb2, branch)
}
}

/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch) {
guard.controls(bb, branch)
}

/** Gets an immediate conditional successor of basic block `bb`, if any. */
BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch) {
result = bb.(Guards::ConditionBlock).getTestSuccessor(branch)
}
}

private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
22 changes: 13 additions & 9 deletions javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,19 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig {
class Guard extends js::ControlFlowNode {
Guard() { this = any(js::ConditionGuardNode g).getTest() }

predicate hasCfgNode(js::BasicBlock bb, int i) { this = bb.getNode(i) }
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(js::BasicBlock bb1, js::BasicBlock bb2, boolean branch) {
exists(js::ConditionGuardNode g |
g.getTest() = this and
bb1 = this.getBasicBlock() and
bb2 = g.getBasicBlock() and
branch = g.getOutcome()
)
}
}

pragma[inline]
Expand All @@ -92,14 +104,6 @@ module SsaDataflowInput implements DataFlowIntegrationInputSig {
branch = g.getOutcome()
)
}

js::BasicBlock getAConditionalBasicBlockSuccessor(js::BasicBlock bb, boolean branch) {
exists(js::ConditionGuardNode g |
bb = g.getTest().getBasicBlock() and
result = g.getBasicBlock() and
branch = g.getOutcome()
)
}
}

import DataFlowIntegration<SsaDataflowInput>
16 changes: 8 additions & 8 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {

/** Gets the SSA definition node corresponding to parameter `p`. */
pragma[nomagic]
SsaImpl::DefinitionExt getParameterDef(NamedParameter p) {
Ssa::Definition getParameterDef(NamedParameter p) {
exists(BasicBlock bb, int i |
bb.getNode(i).getAstNode() = p.getDefiningAccess() and
result.definesAt(_, bb, i, _)
result.definesAt(_, bb, i)
)
}

Expand Down Expand Up @@ -388,15 +388,15 @@ module VariableCapture {
Flow::clearsContent(asClosureNode(node), c.getVariable())
}

class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {
CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable }
class CapturedSsaDefinition extends SsaImpl::Definition {
CapturedSsaDefinition() { this.getSourceVariable() instanceof CapturedVariable }
}

// From an assignment or implicit initialization of a captured variable to its flow-insensitive node
private predicate flowInsensitiveWriteStep(
SsaDefinitionNodeImpl node1, CapturedVariableNode node2, CapturedVariable v
) {
exists(CapturedSsaDefinitionExt def |
exists(CapturedSsaDefinition def |
def = node1.getDefinition() and
def.getSourceVariable() = v and
(
Expand All @@ -412,7 +412,7 @@ module VariableCapture {
private predicate flowInsensitiveReadStep(
CapturedVariableNode node1, SsaDefinitionNodeImpl node2, CapturedVariable v
) {
exists(CapturedSsaDefinitionExt def |
exists(CapturedSsaDefinition def |
node1.getVariable() = v and
def = node2.getDefinition() and
def.getSourceVariable() = v and
Expand Down Expand Up @@ -772,7 +772,7 @@ class SsaNode extends NodeImpl, TSsaNode {
class SsaDefinitionNodeImpl extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;

SsaImpl::Definition getDefinition() { result = node.getDefinition() }
Ssa::Definition getDefinition() { result = node.getDefinition() }

override predicate isHidden() {
exists(SsaImpl::Definition def | def = this.getDefinition() |
Expand Down Expand Up @@ -2478,7 +2478,7 @@ module TypeInference {
n = def or
n.asExpr() =
any(CfgNodes::ExprCfgNode read |
read = def.getDefinition().(SsaImpl::DefinitionExt).getARead() and
read = def.getDefinition().getARead() and
not isTypeCheckedRead(read, _) // could in principle be checked against a new type
)
)
Expand Down
29 changes: 16 additions & 13 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ import Cached
*
* Only intended for internal use.
*/
class DefinitionExt extends Impl::DefinitionExt {
deprecated class DefinitionExt extends Impl::DefinitionExt {
Copy link
Contributor

@hvitved hvitved Mar 6, 2025

Choose a reason for hiding this comment

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

Since these are inside an internal folder, and has the Only intended for internal use comment, it should be fine to simply delete.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

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

They're actually used in a couple of deprecated predicates that ultimately end up in the public getALastRead member predicate on Definition in the outwards-facing SSA.qll, so it's less clear cut. That's why I left them in for now.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

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

We can make it private, though, and possibly push in enough casts from DefinitionExt to Definition that we might be able to eliminate it. Let me check.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

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

Right. All the uses of DefinitionExt here were actually in contexts for which we could push a context with a cast to Definition. So I'll be able to delete this. Commit incoming.

VariableReadAccessCfgNode getARead() { result = getARead(this) }

override string toString() { result = this.(Ssa::Definition).toString() }
Expand All @@ -434,7 +434,7 @@ class DefinitionExt extends Impl::DefinitionExt {
*
* Only intended for internal use.
*/
class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
deprecated class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
override string toString() { result = "SSA phi read(" + this.getSourceVariable() + ")" }

override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() }
Expand All @@ -453,10 +453,10 @@ class NormalParameter extends Parameter {

/** Gets the SSA definition node corresponding to parameter `p`. */
pragma[nomagic]
DefinitionExt getParameterDef(NamedParameter p) {
Definition getParameterDef(NamedParameter p) {
exists(Cfg::BasicBlock bb, int i |
bb.getNode(i).getAstNode() = p.getDefiningAccess() and
result.definesAt(_, bb, i, _)
result.definesAt(_, bb, i)
)
}

Expand Down Expand Up @@ -515,21 +515,24 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) }

class Guard extends Cfg::CfgNodes::AstCfgNode {
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
this.getBasicBlock() = bb1 and
bb2 = bb1.getASuccessor(s) and
s.getValue() = branch
)
}
}

/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
Guards::guardControlsBlock(guard, bb, branch)
}

/** Gets an immediate conditional successor of basic block `bb`, if any. */
SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
exists(Cfg::SuccessorTypes::ConditionalSuccessor s |
result = bb.getASuccessor(s) and
s.getValue() = branch
)
}
}

private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
21 changes: 12 additions & 9 deletions rust/ql/lib/codeql/rust/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,18 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
}

class Guard extends CfgNodes::AstCfgNode {
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch) {
exists(Cfg::ConditionalSuccessor s |
this = bb1.getANode() and
bb2 = bb1.getASuccessor(s) and
s.getValue() = branch
)
}
}

/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
Expand All @@ -372,14 +383,6 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
conditionBlock.edgeDominates(bb, s)
)
}

/** Gets an immediate conditional successor of basic block `bb`, if any. */
SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) {
exists(Cfg::ConditionalSuccessor s |
result = bb.getASuccessor(s) and
s.getValue() = branch
)
}
}

private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
17 changes: 7 additions & 10 deletions shared/ssa/codeql/ssa/Ssa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1434,15 +1434,16 @@ module Make<LocationSig Location, InputSig<Location> Input> {
/** Gets a textual representation of this guard. */
string toString();

/** Holds if the `i`th node of basic block `bb` evaluates this guard. */
predicate hasCfgNode(BasicBlock bb, int i);
/**
* Holds if the control flow branching from `bb1` is dependent on this guard,
* and that the edge from `bb1` to `bb2` corresponds to the evaluation of this
* guard to `branch`.
*/
predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch);
}

/** Holds if `guard` controls block `bb` upon evaluating to `branch`. */
predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch);

/** Gets an immediate conditional successor of basic block `bb`, if any. */
BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch);
}

/**
Expand Down Expand Up @@ -1891,11 +1892,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
|
DfInput::guardControlsBlock(g, bb, branch)
or
exists(int last |
last = bb.length() - 1 and
g.hasCfgNode(bb, last) and
DfInput::getAConditionalBasicBlockSuccessor(bb, branch) = phi.getBasicBlock()
)
g.controlsBranchEdge(bb, phi.getBasicBlock(), branch)
)
)
}
Expand Down
Loading