Skip to content

Commit 709d36b

Browse files
aschackmullaschackmull
authored
Merge pull request #18869 from aschackmull/ssa/refactor3
Ssa: Update qltests including consistency checks
2 parents 795a2e1 + 6c89602 commit 709d36b

19 files changed

Lines changed: 263 additions & 331 deletions

File tree

‎csharp/ql/consistency-queries/SsaConsistency.ql‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,22 +3,6 @@ import semmle.code.csharp.dataflow.internal.SsaImpl as Impl
33
import Impl::Consistency
44
import Ssa
55

6-
class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition {
7-
override predicate hasLocationInfo(
8-
string filepath, int startline, int startcolumn, int endline, int endcolumn
9-
) {
10-
this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
11-
}
12-
}
13-
14-
class MyRelevantDefinitionExt extends RelevantDefinitionExt, Impl::DefinitionExt {
15-
override predicate hasLocationInfo(
16-
string filepath, int startline, int startcolumn, int endline, int endcolumn
17-
) {
18-
this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
19-
}
20-
}
21-
226
query predicate localDeclWithSsaDef(LocalVariableDeclExpr d) {
237
// Local variables in C# must be initialized before every use, so uninitialized
248
// local variables should not have an SSA definition, as that would imply that

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -456,9 +456,9 @@ module VariableCapture {
456456
Flow::clearsContent(asClosureNode(node), getCapturedVariableContent(c))
457457
}
458458

459-
class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {
460-
CapturedSsaDefinitionExt() {
461-
this.getSourceVariable().getAssignable() = any(CapturedVariable v).asLocalScopeVariable()
459+
class CapturedSsaSourceVariable extends Ssa::SourceVariable {
460+
CapturedSsaSourceVariable() {
461+
this.getAssignable() = any(CapturedVariable v).asLocalScopeVariable()
462462
}
463463
}
464464

@@ -509,12 +509,12 @@ module SsaFlow {
509509
result.(Impl::ParameterNode).getParameter() = n.(ExplicitParameterNode).getSsaDefinition()
510510
}
511511

512-
predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
513-
Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
512+
predicate localFlowStep(Ssa::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
513+
Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
514514
}
515515

516-
predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
517-
Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
516+
predicate localMustFlowStep(Ssa::SourceVariable v, Node nodeFrom, Node nodeTo) {
517+
Impl::localMustFlowStep(v, asNode(nodeFrom), asNode(nodeTo))
518518
}
519519
}
520520

@@ -644,12 +644,10 @@ module LocalFlow {
644644
}
645645

646646
/**
647-
* Holds if the source variable of SSA definition `def` is an instance field.
647+
* Holds if the source variable `v` is an instance field.
648648
*/
649-
predicate usesInstanceField(SsaImpl::DefinitionExt def) {
650-
exists(Ssa::SourceVariables::FieldOrPropSourceVariable fp | fp = def.getSourceVariable() |
651-
not fp.getAssignable().(Modifiable).isStatic()
652-
)
649+
predicate isInstanceField(Ssa::SourceVariables::FieldOrPropSourceVariable v) {
650+
not v.getAssignable().(Modifiable).isStatic()
653651
}
654652

655653
predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) {
@@ -749,10 +747,10 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo, string model) {
749747
(
750748
LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
751749
or
752-
exists(SsaImpl::DefinitionExt def, boolean isUseStep |
753-
SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and
754-
not LocalFlow::usesInstanceField(def) and
755-
not def instanceof VariableCapture::CapturedSsaDefinitionExt
750+
exists(Ssa::SourceVariable v, boolean isUseStep |
751+
SsaFlow::localFlowStep(v, nodeFrom, nodeTo, isUseStep) and
752+
not LocalFlow::isInstanceField(v) and
753+
not v instanceof VariableCapture::CapturedSsaSourceVariable
756754
|
757755
isUseStep = false
758756
or
@@ -3007,13 +3005,13 @@ private predicate delegateCreationStep(Node nodeFrom, Node nodeTo) {
30073005

30083006
/** Extra data-flow steps needed for lambda flow analysis. */
30093007
predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preservesValue) {
3010-
exists(SsaImpl::DefinitionExt def |
3011-
SsaFlow::localFlowStep(def, nodeFrom, nodeTo, _) and
3008+
exists(Ssa::SourceVariable v |
3009+
SsaFlow::localFlowStep(v, nodeFrom, nodeTo, _) and
30123010
preservesValue = true
30133011
|
3014-
LocalFlow::usesInstanceField(def)
3012+
LocalFlow::isInstanceField(v)
30153013
or
3016-
def instanceof VariableCapture::CapturedSsaDefinitionExt
3014+
v instanceof VariableCapture::CapturedSsaSourceVariable
30173015
)
30183016
or
30193017
delegateCreationStep(nodeFrom, nodeTo) and

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll‎

Lines changed: 7 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,6 @@ class PhiNode = Impl::PhiNode;
6060

6161
module Consistency = Impl::Consistency;
6262

63-
module ExposedForTestingOnly {
64-
predicate ssaDefReachesReadExt = Impl::ssaDefReachesReadExt/4;
65-
66-
predicate phiHasInputFromBlockExt = Impl::phiHasInputFromBlockExt/3;
67-
}
68-
6963
/**
7064
* Holds if the `i`th node of basic block `bb` reads source variable `v`.
7165
*/
@@ -967,13 +961,13 @@ private module Cached {
967961
import DataFlowIntegrationImpl
968962

969963
cached
970-
predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
971-
DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
964+
predicate localFlowStep(Ssa::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
965+
DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
972966
}
973967

974968
cached
975-
predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) {
976-
DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo)
969+
predicate localMustFlowStep(Ssa::SourceVariable v, Node nodeFrom, Node nodeTo) {
970+
DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
977971
}
978972

979973
signature predicate guardChecksSig(Guards::Guard g, Expr e, Guards::AbstractValue v);
@@ -1000,9 +994,9 @@ private module Cached {
1000994

1001995
import Cached
1002996

1003-
private string getSplitString(DefinitionExt def) {
997+
private string getSplitString(Definition def) {
1004998
exists(ControlFlow::BasicBlock bb, int i, ControlFlow::Node cfn |
1005-
def.definesAt(_, bb, i, _) and
999+
def.definesAt(_, bb, i) and
10061000
result = cfn.(ControlFlow::Nodes::ElementNode).getSplitsString()
10071001
|
10081002
cfn = bb.getNode(i)
@@ -1012,48 +1006,13 @@ private string getSplitString(DefinitionExt def) {
10121006
)
10131007
}
10141008

1015-
string getToStringPrefix(DefinitionExt def) {
1009+
string getToStringPrefix(Definition def) {
10161010
result = "[" + getSplitString(def) + "] "
10171011
or
10181012
not exists(getSplitString(def)) and
10191013
result = ""
10201014
}
10211015

1022-
/**
1023-
* An extended static single assignment (SSA) definition.
1024-
*
1025-
* This is either a normal SSA definition (`Definition`) or a
1026-
* phi-read node (`PhiReadNode`).
1027-
*
1028-
* Only intended for internal use.
1029-
*/
1030-
class DefinitionExt extends Impl::DefinitionExt {
1031-
override string toString() { result = this.(Ssa::Definition).toString() }
1032-
1033-
/** Gets the location of this definition. */
1034-
override Location getLocation() { result = this.(Ssa::Definition).getLocation() }
1035-
1036-
/** Gets the enclosing callable of this definition. */
1037-
Callable getEnclosingCallable() { result = this.(Ssa::Definition).getEnclosingCallable() }
1038-
}
1039-
1040-
/**
1041-
* A phi-read node.
1042-
*
1043-
* Only intended for internal use.
1044-
*/
1045-
class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
1046-
override string toString() {
1047-
result = getToStringPrefix(this) + "SSA phi read(" + this.getSourceVariable() + ")"
1048-
}
1049-
1050-
override Location getLocation() { result = this.getBasicBlock().getLocation() }
1051-
1052-
override Callable getEnclosingCallable() {
1053-
result = this.getSourceVariable().getEnclosingCallable()
1054-
}
1055-
}
1056-
10571016
private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
10581017
private import csharp as Cs
10591018
private import semmle.code.csharp.controlflow.BasicBlocks

‎csharp/ql/test/library-tests/dataflow/ssa/SSAPhiRead.expected‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ phiReadNode
66
| Test.cs:25:16:25:16 | SSA phi read(x) | Test.cs:8:13:8:13 | x |
77
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:78:13:78:13 | x |
88
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:78:13:78:13 | x |
9-
phiReadNodeRead
9+
phiReadNodeFirstRead
1010
| DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) | DefUse.cs:63:9:63:14 | this.Field2 | DefUse.cs:80:37:80:42 | access to field Field2 |
1111
| Fields.cs:63:16:63:28 | SSA phi read(this.LoopField) | Fields.cs:65:24:65:32 | this.LoopField | Fields.cs:65:24:65:32 | access to field LoopField |
1212
| Patterns.cs:20:9:38:9 | SSA phi read(o) | Patterns.cs:7:16:7:16 | o | Patterns.cs:20:17:20:17 | access to local variable o |
@@ -15,16 +15,21 @@ phiReadNodeRead
1515
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:78:13:78:13 | x | Test.cs:92:17:92:17 | access to local variable x |
1616
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:78:13:78:13 | x | Test.cs:96:17:96:17 | access to local variable x |
1717
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:78:13:78:13 | x | Test.cs:99:13:99:13 | access to local variable x |
18-
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:78:13:78:13 | x | Test.cs:104:17:104:17 | access to local variable x |
1918
phiReadInput
20-
| DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) | DefUse.cs:63:9:63:18 | SSA def(this.Field2) |
21-
| DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) | DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) |
19+
| DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) | DefUse.cs:64:13:64:18 | SSA read(this.Field2) |
20+
| DefUse.cs:80:30:80:31 | SSA phi read(this.Field2) | DefUse.cs:80:37:80:42 | SSA read(this.Field2) |
2221
| Fields.cs:63:16:63:28 | SSA phi read(this.LoopField) | Fields.cs:61:17:61:17 | SSA entry def(this.LoopField) |
23-
| Fields.cs:63:16:63:28 | SSA phi read(this.LoopField) | Fields.cs:63:16:63:28 | SSA phi read(this.LoopField) |
24-
| Patterns.cs:20:9:38:9 | SSA phi read(o) | Patterns.cs:7:16:7:23 | SSA def(o) |
22+
| Fields.cs:63:16:63:28 | SSA phi read(this.LoopField) | Fields.cs:65:24:65:32 | SSA read(this.LoopField) |
23+
| Patterns.cs:20:9:38:9 | SSA phi read(o) | Patterns.cs:8:13:8:13 | SSA read(o) |
24+
| Patterns.cs:20:9:38:9 | SSA phi read(o) | Patterns.cs:12:18:12:18 | SSA read(o) |
25+
| Patterns.cs:20:9:38:9 | SSA phi read(o) | Patterns.cs:16:18:16:18 | SSA read(o) |
2526
| Properties.cs:63:16:63:16 | SSA phi read(this.LoopProp) | Properties.cs:61:17:61:17 | SSA entry def(this.LoopProp) |
26-
| Properties.cs:63:16:63:16 | SSA phi read(this.LoopProp) | Properties.cs:63:16:63:16 | SSA phi read(this.LoopProp) |
27+
| Properties.cs:63:16:63:16 | SSA phi read(this.LoopProp) | Properties.cs:65:24:65:31 | SSA read(this.LoopProp) |
2728
| Test.cs:25:16:25:16 | SSA phi read(x) | Test.cs:24:9:24:15 | SSA phi(x) |
28-
| Test.cs:25:16:25:16 | SSA phi read(x) | Test.cs:25:16:25:16 | SSA phi read(x) |
29+
| Test.cs:25:16:25:16 | SSA phi read(x) | Test.cs:25:16:25:16 | SSA read(x) |
2930
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:78:13:78:17 | SSA def(x) |
31+
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:82:17:82:17 | SSA read(x) |
32+
| Test.cs:90:9:97:9 | SSA phi read(x) | Test.cs:86:17:86:17 | SSA read(x) |
3033
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:90:9:97:9 | SSA phi read(x) |
34+
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:92:17:92:17 | SSA read(x) |
35+
| Test.cs:99:9:99:15 | SSA phi read(x) | Test.cs:96:17:96:17 | SSA read(x) |

‎csharp/ql/test/library-tests/dataflow/ssa/SSAPhiRead.ql‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,21 @@
11
import csharp
22
import semmle.code.csharp.dataflow.internal.SsaImpl
3-
import ExposedForTestingOnly
3+
import Impl::TestAdjacentRefs as RefTest
44

5-
query predicate phiReadNode(PhiReadNode phi, Ssa::SourceVariable v) { phi.getSourceVariable() = v }
5+
query predicate phiReadNode(RefTest::Ref phi, Ssa::SourceVariable v) {
6+
phi.isPhiRead() and phi.getSourceVariable() = v
7+
}
68

7-
query predicate phiReadNodeRead(PhiReadNode phi, Ssa::SourceVariable v, ControlFlow::Node read) {
8-
phi.getSourceVariable() = v and
9-
exists(ControlFlow::BasicBlock bb, int i |
10-
ssaDefReachesReadExt(v, phi, bb, i) and
9+
query predicate phiReadNodeFirstRead(RefTest::Ref phi, Ssa::SourceVariable v, ControlFlow::Node read) {
10+
exists(RefTest::Ref r, ControlFlow::BasicBlock bb, int i |
11+
phi.isPhiRead() and
12+
RefTest::adjacentRefRead(phi, r) and
13+
r.accessAt(bb, i, v) and
1114
read = bb.getNode(i)
1215
)
1316
}
1417

15-
query predicate phiReadInput(PhiReadNode phi, DefinitionExt inp) {
16-
phiHasInputFromBlockExt(phi, inp, _)
18+
query predicate phiReadInput(RefTest::Ref phi, RefTest::Ref inp) {
19+
phi.isPhiRead() and
20+
RefTest::adjacentRefPhi(inp, phi)
1721
}

‎java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,12 @@ module SsaFlow {
3636
TExplicitParameterNode(result.(Impl::ParameterNode).getParameter()) = n
3737
}
3838

39-
predicate localFlowStep(
40-
SsaImpl::Impl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep
41-
) {
42-
Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
39+
predicate localFlowStep(SsaSourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
40+
Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
4341
}
4442

45-
predicate localMustFlowStep(SsaImpl::Impl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
46-
Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
43+
predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
44+
Impl::localMustFlowStep(_, asNode(nodeFrom), asNode(nodeTo))
4745
}
4846
}
4947

‎java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ predicate localMustFlowStep(Node node1, Node node2) {
168168
node2.(ImplicitInstanceAccess).getInstanceAccess().(OwnInstanceAccess).getEnclosingCallable()
169169
)
170170
or
171-
SsaFlow::localMustFlowStep(_, node1, node2)
171+
SsaFlow::localMustFlowStep(node1, node2)
172172
or
173173
node2.asExpr().(CastingExpr).getExpr() = node1.asExpr()
174174
or

‎java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -544,15 +544,13 @@ private module Cached {
544544
import DataFlowIntegrationImpl
545545

546546
cached
547-
predicate localFlowStep(Impl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
548-
not def instanceof UntrackedDef and
549-
DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
547+
predicate localFlowStep(TrackedVar v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
548+
DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
550549
}
551550

552551
cached
553-
predicate localMustFlowStep(Impl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
554-
not def instanceof UntrackedDef and
555-
DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo)
552+
predicate localMustFlowStep(TrackedVar v, Node nodeFrom, Node nodeTo) {
553+
DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
556554
}
557555

558556
signature predicate guardChecksSig(Guards::Guard g, Expr e, boolean branch);

‎ruby/ql/consistency-queries/SsaConsistency.ql‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,3 @@
11
import codeql.ruby.dataflow.SSA
22
import codeql.ruby.dataflow.internal.SsaImpl
33
import Consistency
4-
5-
class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition {
6-
override predicate hasLocationInfo(
7-
string filepath, int startline, int startcolumn, int endline, int endcolumn
8-
) {
9-
this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
10-
}
11-
}
12-
13-
class MyRelevantDefinitionExt extends RelevantDefinitionExt, DefinitionExt {
14-
override predicate hasLocationInfo(
15-
string filepath, int startline, int startcolumn, int endline, int endcolumn
16-
) {
17-
this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
18-
}
19-
}

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,14 @@ module SsaFlow {
111111
n = toParameterNode(result.(Impl::ParameterNode).getParameter())
112112
}
113113

114-
predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
115-
Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
114+
predicate localFlowStep(
115+
SsaImpl::SsaInput::SourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep
116+
) {
117+
Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
116118
}
117119

118-
predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
119-
Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
120+
predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
121+
Impl::localMustFlowStep(_, asNode(nodeFrom), asNode(nodeTo))
120122
}
121123
}
122124

@@ -175,7 +177,7 @@ module LocalFlow {
175177
}
176178

177179
predicate localMustFlowStep(Node node1, Node node2) {
178-
SsaFlow::localMustFlowStep(_, node1, node2)
180+
SsaFlow::localMustFlowStep(node1, node2)
179181
or
180182
node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
181183
or
@@ -525,10 +527,10 @@ private module Cached {
525527
(
526528
LocalFlow::localFlowStepCommon(nodeFrom, nodeTo)
527529
or
528-
exists(SsaImpl::DefinitionExt def, boolean isUseStep |
529-
SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and
530+
exists(SsaImpl::SsaInput::SourceVariable v, boolean isUseStep |
531+
SsaFlow::localFlowStep(v, nodeFrom, nodeTo, isUseStep) and
530532
// captured variables are handled by the shared `VariableCapture` library
531-
not def instanceof VariableCapture::CapturedSsaDefinitionExt
533+
not v instanceof VariableCapture::CapturedVariable
532534
|
533535
isUseStep = false
534536
or

0 commit comments

Comments
 (0)