From 133f08784fa738aa4ddf3711d505f8bd132ab80c Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 20 Mar 2025 13:12:48 +0100 Subject: [PATCH 1/3] C++: Eliminate dead code, uncertain is always false. --- .../ir/dataflow/internal/DataFlowPrivate.qll | 44 ++++--------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index f6371e5b696c..e3e07c1522ce 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -1879,15 +1879,6 @@ module IteratorFlow { phi.definesAt(sv, bb2, i2, _) ) } - - cached - Node getAPriorDefinition(IteratorSsa::DefinitionExt next) { - exists(IRBlock bb, int i, SourceVariable sv, IteratorSsa::DefinitionExt def | - IteratorSsa::lastRefRedefExt(pragma[only_bind_into](def), pragma[only_bind_into](sv), - pragma[only_bind_into](bb), pragma[only_bind_into](i), next) and - nodeToDefOrUse(result, sv, bb, i, _) - ) - } } /** The set of nodes necessary for iterator flow. */ @@ -1912,25 +1903,19 @@ module IteratorFlow { private import IteratorSsaCached - private predicate defToNode(Node node, Def def, boolean uncertain) { - ( - nodeHasOperand(node, def.getValue().asOperand(), def.getIndirectionIndex()) - or - nodeHasInstruction(node, def.getValue().asInstruction(), def.getIndirectionIndex()) - ) and - uncertain = false + private predicate defToNode(Node node, Def def) { + nodeHasOperand(node, def.getValue().asOperand(), def.getIndirectionIndex()) + or + nodeHasInstruction(node, def.getValue().asInstruction(), def.getIndirectionIndex()) } - private predicate nodeToDefOrUse( - Node node, SourceVariable sv, IRBlock bb, int i, boolean uncertain - ) { + private predicate nodeToDefOrUse(Node node, SourceVariable sv, IRBlock bb, int i) { exists(Def def | def.hasIndexInBlock(bb, i, sv) and - defToNode(node, def, uncertain) + defToNode(node, def) ) or - useToNode(bb, i, sv, node) and - uncertain = false + useToNode(bb, i, sv, node) } private predicate useToNode(IRBlock bb, int i, SourceVariable sv, Node nodeTo) { @@ -1949,21 +1934,10 @@ module IteratorFlow { * Holds if `nodeFrom` flows to `nodeTo` in a single step. */ predicate localFlowStep(Node nodeFrom, Node nodeTo) { - exists( - Node nFrom, SourceVariable sv, IRBlock bb1, int i1, IRBlock bb2, int i2, boolean uncertain - | + exists(SourceVariable sv, IRBlock bb1, int i1, IRBlock bb2, int i2 | adjacentDefRead(bb1, i1, sv, bb2, i2) and - nodeToDefOrUse(nFrom, sv, bb1, i1, uncertain) and + nodeToDefOrUse(nodeFrom, sv, bb1, i1) and useToNode(bb2, i2, sv, nodeTo) - | - if uncertain = true - then - nodeFrom = - [ - nFrom, - getAPriorDefinition(any(IteratorSsa::DefinitionExt next | next.definesAt(sv, bb1, i1, _))) - ] - else nFrom = nodeFrom ) } } From aaa7e4cf95f00fcad879e187a96ef1f7ef3a5405 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 26 Mar 2025 13:58:58 +0100 Subject: [PATCH 2/3] C++: Def is only used in defToNode, which doesn't include phi reads nodes. --- .../semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index e3e07c1522ce..13243f2b22ea 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -1834,7 +1834,7 @@ module IteratorFlow { private module IteratorSsa = SsaImpl::Make; - private class Def extends IteratorSsa::DefinitionExt { + private class Def extends IteratorSsa::Definition { final override Location getLocation() { result = this.getImpl().getLocation() } /** @@ -1842,7 +1842,7 @@ module IteratorFlow { * and is a definition (or use) of the variable `sv`. */ predicate hasIndexInBlock(IRBlock block, int index, SourceVariable sv) { - super.definesAt(sv, block, index, _) + super.definesAt(sv, block, index) } private Ssa::DefImpl getImpl() { From a6a694dec65b2e086cebc3676ddbadaab9a2ad92 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 20 Mar 2025 14:36:17 +0100 Subject: [PATCH 3/3] C++: Use DataFlowIntegration in IteratorFlow. --- .../ir/dataflow/internal/DataFlowPrivate.qll | 100 ++++++++++-------- .../cpp/ir/dataflow/internal/SsaInternals.qll | 2 +- 2 files changed, 55 insertions(+), 47 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index 13243f2b22ea..8a5155239304 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -1834,6 +1834,46 @@ module IteratorFlow { private module IteratorSsa = SsaImpl::Make; + private module DataFlowIntegrationInput implements IteratorSsa::DataFlowIntegrationInputSig { + private import codeql.util.Void + + class Expr extends Instruction { + Expr() { + exists(IRBlock bb, int i | + SsaInput::variableRead(bb, i, _, true) and + this = bb.getInstruction(i) + ) + } + + predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { bb.getInstruction(i) = this } + } + + predicate ssaDefHasSource(IteratorSsa::WriteDefinition def) { none() } + + predicate allowFlowIntoUncertainDef(IteratorSsa::UncertainWriteDefinition def) { any() } + + class Guard extends Void { + predicate controlsBranchEdge( + SsaInput::BasicBlock bb1, SsaInput::BasicBlock bb2, boolean branch + ) { + none() + } + } + + predicate guardDirectlyControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { + none() + } + + predicate supportBarrierGuardsOnPhiEdges() { none() } + } + + private module DataFlowIntegrationImpl = + IteratorSsa::DataFlowIntegration; + + private class IteratorSynthNode extends DataFlowIntegrationImpl::SsaNode { + IteratorSynthNode() { not this.asDefinition() instanceof IteratorSsa::WriteDefinition } + } + private class Def extends IteratorSsa::Definition { final override Location getLocation() { result = this.getImpl().getLocation() } @@ -1859,37 +1899,15 @@ module IteratorFlow { int getIndirectionIndex() { result = this.getImpl().getIndirectionIndex() } } - private class PhiNode extends IteratorSsa::DefinitionExt { - PhiNode() { - this instanceof IteratorSsa::PhiNode or - this instanceof IteratorSsa::PhiReadNode - } - - SsaIteratorNode getNode() { result.getIteratorFlowNode() = this } - } - - cached - private module IteratorSsaCached { - cached - predicate adjacentDefRead(IRBlock bb1, int i1, SourceVariable sv, IRBlock bb2, int i2) { - IteratorSsa::adjacentDefReadExt(_, sv, bb1, i1, bb2, i2) - or - exists(PhiNode phi | - IteratorSsa::lastRefRedefExt(_, sv, bb1, i1, phi) and - phi.definesAt(sv, bb2, i2, _) - ) - } - } - /** The set of nodes necessary for iterator flow. */ - class IteratorFlowNode instanceof PhiNode { + class IteratorFlowNode instanceof IteratorSynthNode { /** Gets a textual representation of this node. */ string toString() { result = super.toString() } /** Gets the type of this node. */ DataFlowType getType() { exists(Ssa::SourceVariable sv | - super.definesAt(sv, _, _, _) and + super.getSourceVariable() = sv and result = sv.getType() ) } @@ -1901,43 +1919,33 @@ module IteratorFlow { Location getLocation() { result = super.getBasicBlock().getLocation() } } - private import IteratorSsaCached - private predicate defToNode(Node node, Def def) { nodeHasOperand(node, def.getValue().asOperand(), def.getIndirectionIndex()) or nodeHasInstruction(node, def.getValue().asInstruction(), def.getIndirectionIndex()) } - private predicate nodeToDefOrUse(Node node, SourceVariable sv, IRBlock bb, int i) { - exists(Def def | - def.hasIndexInBlock(bb, i, sv) and - defToNode(node, def) - ) + bindingset[result, v] + pragma[inline_late] + private DataFlowIntegrationImpl::Node fromDfNode(Node n, SourceVariable v) { + result = n.(SsaIteratorNode).getIteratorFlowNode() or - useToNode(bb, i, sv, node) - } - - private predicate useToNode(IRBlock bb, int i, SourceVariable sv, Node nodeTo) { - exists(PhiNode phi | - phi.definesAt(sv, bb, i, _) and - nodeTo = phi.getNode() + exists(Ssa::UseImpl use, IRBlock bb, int i | + result.(DataFlowIntegrationImpl::ExprNode).getExpr().hasCfgNode(bb, i) and + use.hasIndexInBlock(bb, i, v) and + use.getNode() = n ) or - exists(Ssa::UseImpl use | - use.hasIndexInBlock(bb, i, sv) and - nodeTo = use.getNode() - ) + defToNode(n, result.(DataFlowIntegrationImpl::SsaDefinitionNode).getDefinition()) } /** * Holds if `nodeFrom` flows to `nodeTo` in a single step. */ predicate localFlowStep(Node nodeFrom, Node nodeTo) { - exists(SourceVariable sv, IRBlock bb1, int i1, IRBlock bb2, int i2 | - adjacentDefRead(bb1, i1, sv, bb2, i2) and - nodeToDefOrUse(nodeFrom, sv, bb1, i1) and - useToNode(bb2, i2, sv, nodeTo) + exists(SourceVariable v | + nodeFrom != nodeTo and + DataFlowIntegrationImpl::localFlowStep(v, fromDfNode(nodeFrom, v), fromDfNode(nodeTo, v), _) ) } } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll index 202d3fa32c80..51829f13df51 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll @@ -1069,7 +1069,7 @@ module BarrierGuard { bindingset[result, v] pragma[inline_late] -DataFlowIntegrationImpl::Node fromDfNode(Node n, SourceVariable v) { +private DataFlowIntegrationImpl::Node fromDfNode(Node n, SourceVariable v) { result = n.(SsaSynthNode).getSynthNode() or exists(UseImpl use, IRBlock bb, int i |