Skip to content

Commit 47b1c3d

Browse files
aschackmullaschackmull
authored
Merge pull request #19154 from aschackmull/ssa/variablecapture
Ssa: Replace phi-read references in VariableCapture with default use-use flow
2 parents 45b55c0 + 56c46d7 commit 47b1c3d

File tree

6 files changed

+103
-90
lines changed

6 files changed

+103
-90
lines changed

‎java/ql/test/library-tests/dataflow/capture/test.expected

-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
| A.java:21:11:21:13 | "B" : String | A.java:15:16:15:22 | get(...) : String |
1919
| A.java:21:11:21:13 | "B" : String | A.java:21:7:21:13 | ...=... : String |
2020
| A.java:21:11:21:13 | "B" : String | A.java:25:5:25:26 | SSA phi(s) : String |
21-
| A.java:21:11:21:13 | "B" : String | A.java:25:5:25:26 | phi(String s) : String |
2221
| A.java:21:11:21:13 | "B" : String | A.java:28:11:38:5 | String s : String |
2322
| A.java:21:11:21:13 | "B" : String | A.java:28:11:38:5 | new (...) : new A(...) { ... } [String s] |
2423
| A.java:21:11:21:13 | "B" : String | A.java:30:14:30:16 | parameter this : new A(...) { ... } [String s] |
@@ -35,7 +34,6 @@
3534
| A.java:23:11:23:13 | "C" : String | A.java:15:16:15:22 | get(...) : String |
3635
| A.java:23:11:23:13 | "C" : String | A.java:23:7:23:13 | ...=... : String |
3736
| A.java:23:11:23:13 | "C" : String | A.java:25:5:25:26 | SSA phi(s) : String |
38-
| A.java:23:11:23:13 | "C" : String | A.java:25:5:25:26 | phi(String s) : String |
3937
| A.java:23:11:23:13 | "C" : String | A.java:28:11:38:5 | String s : String |
4038
| A.java:23:11:23:13 | "C" : String | A.java:28:11:38:5 | new (...) : new A(...) { ... } [String s] |
4139
| A.java:23:11:23:13 | "C" : String | A.java:30:14:30:16 | parameter this : new A(...) { ... } [String s] |

‎rust/ql/test/library-tests/dataflow/local/CONSISTENCY/DataFlowConsistency.expected

-2
This file was deleted.

‎rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected

+4-5
Original file line numberDiff line numberDiff line change
@@ -647,10 +647,9 @@ localStep
647647
| main.rs:441:24:441:33 | [post] receiver for source(...) | main.rs:441:24:441:33 | [post] source(...) |
648648
| main.rs:441:24:441:33 | source(...) | main.rs:441:24:441:33 | receiver for source(...) |
649649
| main.rs:441:24:441:45 | ... .to_string(...) | main.rs:441:9:441:20 | default_name |
650-
| main.rs:441:24:441:45 | ... .to_string(...) | main.rs:442:9:442:20 | phi(default_name) |
650+
| main.rs:441:24:441:45 | ... .to_string(...) | main.rs:442:9:442:20 | SSA phi read(default_name) |
651651
| main.rs:442:5:448:5 | for ... in ... { ... } | main.rs:440:75:449:1 | { ... } |
652-
| main.rs:442:9:442:20 | phi(default_name) | main.rs:442:9:442:20 | phi(default_name) |
653-
| main.rs:442:9:442:20 | phi(default_name) | main.rs:444:41:444:67 | default_name |
652+
| main.rs:442:9:442:20 | SSA phi read(default_name) | main.rs:444:41:444:67 | default_name |
654653
| main.rs:442:10:442:13 | [SSA] cond | main.rs:443:12:443:15 | cond |
655654
| main.rs:442:10:442:13 | cond | main.rs:442:10:442:13 | [SSA] cond |
656655
| main.rs:442:10:442:13 | cond | main.rs:442:10:442:13 | cond |
@@ -664,9 +663,9 @@ localStep
664663
| main.rs:444:21:444:24 | [post] receiver for name | main.rs:444:21:444:24 | [post] name |
665664
| main.rs:444:21:444:24 | name | main.rs:444:21:444:24 | receiver for name |
666665
| main.rs:444:21:444:68 | name.unwrap_or_else(...) | main.rs:444:17:444:17 | n |
667-
| main.rs:444:41:444:67 | [post] default_name | main.rs:442:9:442:20 | phi(default_name) |
666+
| main.rs:444:41:444:67 | [post] default_name | main.rs:442:9:442:20 | SSA phi read(default_name) |
668667
| main.rs:444:41:444:67 | closure self in \|...\| ... | main.rs:444:44:444:55 | this |
669-
| main.rs:444:41:444:67 | default_name | main.rs:442:9:442:20 | phi(default_name) |
668+
| main.rs:444:41:444:67 | default_name | main.rs:442:9:442:20 | SSA phi read(default_name) |
670669
| main.rs:444:44:444:55 | [post] receiver for default_name | main.rs:444:44:444:55 | [post] default_name |
671670
| main.rs:444:44:444:55 | default_name | main.rs:444:44:444:55 | receiver for default_name |
672671
| main.rs:445:18:445:18 | [post] receiver for n | main.rs:445:18:445:18 | [post] n |

‎shared/dataflow/codeql/dataflow/VariableCapture.qll

+83-65
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
699699
class SourceVariable = CaptureContainer;
700700

701701
predicate variableWrite(BasicBlock bb, int i, SourceVariable cc, boolean certain) {
702+
Cached::ref() and
702703
(
703704
exists(CapturedVariable v | cc = TVariable(v) and captureWrite(v, bb, i, true, _))
704705
or
@@ -721,23 +722,55 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
721722

722723
private module CaptureSsa = Ssa::Make<Location, CaptureSsaInput>;
723724

724-
private newtype TClosureNode =
725-
TSynthRead(CapturedVariable v, BasicBlock bb, int i, Boolean isPost) {
726-
synthRead(v, bb, i, _, _)
727-
} or
728-
TSynthThisQualifier(BasicBlock bb, int i, Boolean isPost) { synthThisQualifier(bb, i) } or
729-
TSynthPhi(CaptureSsa::DefinitionExt phi) {
730-
phi instanceof CaptureSsa::PhiNode or phi instanceof CaptureSsa::PhiReadNode
731-
} or
732-
TExprNode(Expr expr, Boolean isPost) {
733-
expr instanceof VariableRead
734-
or
735-
synthRead(_, _, _, _, expr)
736-
} or
737-
TParamNode(CapturedParameter p) or
738-
TThisParamNode(Callable c) { captureAccess(_, c) } or
739-
TMallocNode(ClosureExpr ce) { hasConstructorCapture(ce, _) } or
740-
TVariableWriteSourceNode(VariableWrite write)
725+
private module DataFlowIntegrationInput implements CaptureSsa::DataFlowIntegrationInputSig {
726+
private import codeql.util.Void
727+
728+
class Expr instanceof Input::ControlFlowNode {
729+
string toString() { result = super.toString() }
730+
731+
predicate hasCfgNode(BasicBlock bb, int i) { bb.getNode(i) = this }
732+
}
733+
734+
class Guard extends Void {
735+
predicate controlsBranchEdge(BasicBlock bb1, BasicBlock bb2, boolean branch) { none() }
736+
}
737+
738+
predicate guardDirectlyControlsBlock(Guard guard, BasicBlock bb, boolean branch) { none() }
739+
740+
predicate includeWriteDefsInFlowStep() { none() }
741+
742+
predicate supportBarrierGuardsOnPhiEdges() { none() }
743+
}
744+
745+
private module SsaFlow = CaptureSsa::DataFlowIntegration<DataFlowIntegrationInput>;
746+
747+
cached
748+
private module Cached {
749+
cached
750+
predicate ref() { any() }
751+
752+
cached
753+
predicate backref() { localFlowStep(_, _) implies any() }
754+
755+
cached
756+
newtype TClosureNode =
757+
TSynthRead(CapturedVariable v, BasicBlock bb, int i, Boolean isPost) {
758+
synthRead(v, bb, i, _, _)
759+
} or
760+
TSynthThisQualifier(BasicBlock bb, int i, Boolean isPost) { synthThisQualifier(bb, i) } or
761+
TSynthSsa(SsaFlow::SsaNode n) or
762+
TExprNode(Expr expr, Boolean isPost) {
763+
expr instanceof VariableRead
764+
or
765+
synthRead(_, _, _, _, expr)
766+
} or
767+
TParamNode(CapturedParameter p) or
768+
TThisParamNode(Callable c) { captureAccess(_, c) } or
769+
TMallocNode(ClosureExpr ce) { hasConstructorCapture(ce, _) } or
770+
TVariableWriteSourceNode(VariableWrite write)
771+
}
772+
773+
private import Cached
741774

742775
class ClosureNode extends TClosureNode {
743776
/** Gets a textual representation of this node. */
@@ -746,11 +779,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
746779
or
747780
result = "this" and this = TSynthThisQualifier(_, _, _)
748781
or
749-
exists(CaptureSsa::DefinitionExt phi, CaptureContainer cc |
750-
this = TSynthPhi(phi) and
751-
phi.definesAt(cc, _, _, _) and
752-
result = "phi(" + cc.toString() + ")"
753-
)
782+
exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and result = n.toString())
754783
or
755784
exists(Expr expr, boolean isPost | this = TExprNode(expr, isPost) |
756785
isPost = false and result = expr.toString()
@@ -784,9 +813,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
784813
captureWrite(_, bb, i, false, any(VariableWrite vw | result = vw.getLocation()))
785814
)
786815
or
787-
exists(CaptureSsa::DefinitionExt phi, BasicBlock bb |
788-
this = TSynthPhi(phi) and phi.definesAt(_, bb, _, _) and result = bb.getLocation()
789-
)
816+
exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and result = n.getLocation())
790817
or
791818
exists(Expr expr | this = TExprNode(expr, _) and result = expr.getLocation())
792819
or
@@ -802,35 +829,29 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
802829
}
803830
}
804831

805-
private class TSynthesizedCaptureNode = TSynthRead or TSynthThisQualifier or TSynthPhi;
832+
private class TSynthesizedCaptureNode = TSynthRead or TSynthThisQualifier or TSynthSsa;
806833

807834
class SynthesizedCaptureNode extends ClosureNode, TSynthesizedCaptureNode {
808835
BasicBlock getBasicBlock() {
809836
this = TSynthRead(_, result, _, _)
810837
or
811838
this = TSynthThisQualifier(result, _, _)
812839
or
813-
exists(CaptureSsa::DefinitionExt phi |
814-
this = TSynthPhi(phi) and phi.definesAt(_, result, _, _)
815-
)
840+
exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getBasicBlock() = result)
816841
}
817842

818843
Callable getEnclosingCallable() { result = this.getBasicBlock().getEnclosingCallable() }
819844

820845
predicate isVariableAccess(CapturedVariable v) {
821846
this = TSynthRead(v, _, _, _)
822847
or
823-
exists(CaptureSsa::DefinitionExt phi |
824-
this = TSynthPhi(phi) and phi.definesAt(TVariable(v), _, _, _)
825-
)
848+
exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getSourceVariable() = TVariable(v))
826849
}
827850

828851
predicate isInstanceAccess() {
829852
this instanceof TSynthThisQualifier
830853
or
831-
exists(CaptureSsa::DefinitionExt phi |
832-
this = TSynthPhi(phi) and phi.definesAt(TThis(_), _, _, _)
833-
)
854+
exists(SsaFlow::SsaNode n | this = TSynthSsa(n) and n.getSourceVariable() = TThis(_))
834855
}
835856
}
836857

@@ -872,18 +893,7 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
872893
)
873894
}
874895

875-
private predicate step(CaptureContainer cc, BasicBlock bb1, int i1, BasicBlock bb2, int i2) {
876-
CaptureSsa::adjacentDefReadExt(_, cc, bb1, i1, bb2, i2)
877-
}
878-
879-
private predicate stepToPhi(CaptureContainer cc, BasicBlock bb, int i, TSynthPhi phi) {
880-
exists(CaptureSsa::DefinitionExt next |
881-
CaptureSsa::lastRefRedefExt(_, cc, bb, i, next) and
882-
phi = TSynthPhi(next)
883-
)
884-
}
885-
886-
private predicate ssaAccessAt(
896+
private predicate ssaReadAt(
887897
ClosureNode n, CaptureContainer cc, boolean isPost, BasicBlock bb, int i
888898
) {
889899
exists(CapturedVariable v |
@@ -894,49 +904,57 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
894904
or
895905
n = TSynthThisQualifier(bb, i, isPost) and cc = TThis(bb.getEnclosingCallable())
896906
or
897-
exists(CaptureSsa::DefinitionExt phi |
898-
n = TSynthPhi(phi) and phi.definesAt(cc, bb, i, _) and isPost = false
899-
)
900-
or
901907
exists(VariableRead vr, CapturedVariable v |
902908
captureRead(v, bb, i, true, vr) and
903909
n = TExprNode(vr, isPost) and
904910
cc = TVariable(v)
905911
)
906-
or
912+
}
913+
914+
private predicate ssaWriteAt(ClosureNode n, CaptureContainer cc, BasicBlock bb, int i) {
907915
exists(VariableWrite vw, CapturedVariable v |
908916
captureWrite(v, bb, i, true, vw) and
909917
n = TVariableWriteSourceNode(vw) and
910-
isPost = false and
911918
cc = TVariable(v)
912919
)
913920
or
914921
exists(CapturedParameter p |
915922
entryDef(cc, bb, i) and
916923
cc = TVariable(p) and
917-
n = TParamNode(p) and
918-
isPost = false
924+
n = TParamNode(p)
919925
)
920926
or
921927
exists(Callable c |
922928
entryDef(cc, bb, i) and
923929
cc = TThis(c) and
924-
n = TThisParamNode(c) and
925-
isPost = false
930+
n = TThisParamNode(c)
926931
)
927932
}
928933

929-
predicate localFlowStep(ClosureNode node1, ClosureNode node2) {
930-
exists(CaptureContainer cc, BasicBlock bb1, int i1, BasicBlock bb2, int i2 |
931-
step(cc, bb1, i1, bb2, i2) and
932-
ssaAccessAt(node1, pragma[only_bind_into](cc), _, bb1, i1) and
933-
ssaAccessAt(node2, pragma[only_bind_into](cc), false, bb2, i2)
934+
bindingset[result, cc]
935+
pragma[inline_late]
936+
private SsaFlow::Node asNode(CaptureContainer cc, ClosureNode n) {
937+
n = TSynthSsa(result)
938+
or
939+
exists(BasicBlock bb, int i |
940+
result.(SsaFlow::ExprNode).getExpr().hasCfgNode(bb, i) and
941+
ssaReadAt(n, cc, false, bb, i)
934942
)
935943
or
936-
exists(CaptureContainer cc, BasicBlock bb, int i |
937-
stepToPhi(cc, bb, i, node2) and
938-
ssaAccessAt(node1, cc, _, bb, i)
944+
exists(BasicBlock bb, int i |
945+
result.(SsaFlow::ExprPostUpdateNode).getExpr().hasCfgNode(bb, i) and
946+
ssaReadAt(n, cc, true, bb, i)
939947
)
948+
or
949+
exists(BasicBlock bb, int i |
950+
result.(SsaFlow::WriteDefSourceNode).getDefinition().definesAt(cc, bb, i) and
951+
ssaWriteAt(n, cc, bb, i)
952+
)
953+
}
954+
955+
cached
956+
predicate localFlowStep(ClosureNode n1, ClosureNode n2) {
957+
exists(CaptureContainer cc | SsaFlow::localFlowStep(cc, asNode(cc, n1), asNode(cc, n2), _))
940958
}
941959

942960
private predicate storeStepClosure(

‎shared/ssa/codeql/ssa/Ssa.qll

+11-11
Original file line numberDiff line numberDiff line change
@@ -1661,7 +1661,16 @@ module Make<LocationSig Location, InputSig<Location> Input> {
16611661
private newtype TNode =
16621662
TWriteDefSource(WriteDefinition def) { DfInput::ssaDefHasSource(def) } or
16631663
TExprNode(DfInput::Expr e, Boolean isPost) { e = DfInput::getARead(_) } or
1664-
TSsaDefinitionNode(DefinitionExt def) { not phiHasUniqNextNode(def) } or
1664+
TSsaDefinitionNode(DefinitionExt def) {
1665+
not phiHasUniqNextNode(def) and
1666+
if DfInput::includeWriteDefsInFlowStep()
1667+
then any()
1668+
else (
1669+
def instanceof PhiNode or
1670+
def instanceof PhiReadNode or
1671+
DfInput::allowFlowIntoUncertainDef(def)
1672+
)
1673+
} or
16651674
TSsaInputNode(SsaPhiExt phi, BasicBlock input) { relevantPhiInputNode(phi, input) }
16661675

16671676
/**
@@ -1748,8 +1757,6 @@ module Make<LocationSig Location, InputSig<Location> Input> {
17481757
this.getExpr().hasCfgNode(bb_, i_)
17491758
}
17501759

1751-
SourceVariable getVariable() { result = v_ }
1752-
17531760
pragma[nomagic]
17541761
predicate readsAt(BasicBlock bb, int i, SourceVariable v) {
17551762
bb = bb_ and
@@ -1904,14 +1911,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
19041911
exists(DefinitionExt def |
19051912
nodeFrom.(SsaDefinitionExtNodeImpl).getDefExt() = def and
19061913
def.definesAt(v, bb, i, _) and
1907-
isUseStep = false and
1908-
if DfInput::includeWriteDefsInFlowStep()
1909-
then any()
1910-
else (
1911-
def instanceof PhiNode or
1912-
def instanceof PhiReadNode or
1913-
DfInput::allowFlowIntoUncertainDef(def)
1914-
)
1914+
isUseStep = false
19151915
)
19161916
or
19171917
[nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()].(ReadNode).readsAt(bb, i, v) and

‎swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected

+5-5
Original file line numberDiff line numberDiff line change
@@ -1378,17 +1378,17 @@
13781378
| test.swift:888:9:888:9 | stream | test.swift:888:9:888:9 | SSA def(stream) |
13791379
| test.swift:888:18:896:6 | call to AsyncStream<Element>.init(_:bufferingPolicy:_:) | test.swift:888:9:888:9 | stream |
13801380
| test.swift:889:9:889:9 | continuation | test.swift:890:27:895:13 | continuation |
1381-
| test.swift:890:27:895:13 | closure self parameter | test.swift:891:17:891:17 | phi(this) |
1381+
| test.swift:890:27:895:13 | closure self parameter | test.swift:891:17:891:17 | SSA phi read(this) |
13821382
| test.swift:891:17:891:17 | $generator | test.swift:891:17:891:17 | &... |
13831383
| test.swift:891:17:891:17 | &... | test.swift:891:17:891:17 | $generator |
1384+
| test.swift:891:17:891:17 | SSA phi read(this) | test.swift:892:21:892:21 | this |
1385+
| test.swift:891:17:891:17 | SSA phi read(this) | test.swift:894:17:894:17 | this |
13841386
| test.swift:891:17:891:17 | [post] $generator | test.swift:891:17:891:17 | &... |
1385-
| test.swift:891:17:891:17 | phi(this) | test.swift:892:21:892:21 | this |
1386-
| test.swift:891:17:891:17 | phi(this) | test.swift:894:17:894:17 | this |
13871387
| test.swift:891:26:891:26 | $generator | test.swift:891:26:891:26 | SSA def($generator) |
13881388
| test.swift:891:26:891:26 | SSA def($generator) | test.swift:891:17:891:17 | $generator |
13891389
| test.swift:891:26:891:30 | call to makeIterator() | test.swift:891:26:891:26 | $generator |
1390-
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
1391-
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | phi(this) |
1390+
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | SSA phi read(this) |
1391+
| test.swift:892:21:892:21 | this | test.swift:891:17:891:17 | SSA phi read(this) |
13921392
| test.swift:898:5:898:5 | $i$generator | test.swift:898:5:898:5 | &... |
13931393
| test.swift:898:5:898:5 | &... | test.swift:898:5:898:5 | $i$generator |
13941394
| test.swift:898:5:898:5 | [post] $i$generator | test.swift:898:5:898:5 | &... |

0 commit comments

Comments
 (0)