Skip to content

Commit 83d204d

Browse files
hvitvedhvitved
authored
Merge pull request #7218 from hvitved/ssa/fix-consistency-tests
Ruby: Fix SSA consistency tests + CFG bug
2 parents 3e1164f + 4d918b5 commit 83d204d

File tree

9 files changed

+64
-43
lines changed

9 files changed

+64
-43
lines changed

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

‎csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

‎csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

‎ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,18 @@ module EnsureSplitting {
139139

140140
/** Holds if this node is the entry node in the `ensure` block it belongs to. */
141141
predicate isEntryNode() { first(block.getEnsure(), this) }
142+
143+
BodyStmt getBlock() { result = block }
144+
145+
pragma[noinline]
146+
predicate isEntered(AstNode pred, int nestLevel, Completion c) {
147+
this.isEntryNode() and
148+
nestLevel = this.getNestLevel() and
149+
succ(pred, this, c) and
150+
// the entry node may be reachable via a backwards loop edge; in this case
151+
// the split has already been entered
152+
not pred = block.getAnEnsureDescendant()
153+
}
142154
}
143155

144156
/**
@@ -205,18 +217,11 @@ module EnsureSplitting {
205217
override string toString() { result = "ensure (" + nestLevel + ")" }
206218
}
207219

208-
pragma[noinline]
209-
private predicate hasEntry0(AstNode pred, EnsureNode succ, int nestLevel, Completion c) {
210-
succ.isEntryNode() and
211-
nestLevel = succ.getNestLevel() and
212-
succ(pred, succ, c)
213-
}
214-
215220
private class EnsureSplitImpl extends SplitImpl, EnsureSplit {
216221
override EnsureSplitKind getKind() { result.getNestLevel() = this.getNestLevel() }
217222

218223
override predicate hasEntry(AstNode pred, AstNode succ, Completion c) {
219-
hasEntry0(pred, succ, this.getNestLevel(), c) and
224+
succ.(EnsureNode).isEntered(pred, this.getNestLevel(), c) and
220225
this.getType().isSplitForEntryCompletion(c)
221226
}
222227

@@ -323,7 +328,7 @@ module EnsureSplitting {
323328
succ(pred, succ, c) and
324329
succ =
325330
any(EnsureNode en |
326-
if en.isEntryNode()
331+
if en.isEntryNode() and en.getBlock() != pred.(EnsureNode).getBlock()
327332
then
328333
// entering a nested `ensure` block
329334
en.getNestLevel() > this.getNestLevel()

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -645,17 +645,18 @@ module Consistency {
645645

646646
query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
647647
ssaDefReachesRead(v, def, bb, i) and
648-
not exists(unique(Definition def0 | ssaDefReachesRead(_, def0, bb, i)))
648+
not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
649649
}
650650

651651
query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
652652
variableRead(bb, i, v, _) and
653-
not ssaDefReachesRead(_, _, bb, i)
653+
not ssaDefReachesRead(v, _, bb, i)
654654
}
655655

656656
query predicate deadDef(RelevantDefinition def, SourceVariable v) {
657657
v = def.getSourceVariable() and
658658
not ssaDefReachesRead(_, def, _, _) and
659-
not phiHasInputFromBlock(_, def, _)
659+
not phiHasInputFromBlock(_, def, _) and
660+
not uncertainWriteDefinitionInput(_, def)
660661
}
661662
}

‎ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,10 +322,13 @@ break_ensure.rb:
322322
#-----| -> [ensure: raise] ... < ...
323323

324324
# 33| do ...
325+
#-----| -> y
325326

326327
# 33| [ensure: return] do ...
328+
#-----| -> [ensure: return] y
327329

328330
# 33| [ensure: raise] do ...
331+
#-----| -> [ensure: raise] y
329332

330333
# 35| if ...
331334
#-----| -> do ...

‎ruby/ql/test/library-tests/controlflow/graph/Nodes.expected

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
callsWithNoArguments
2+
| break_ensure.rb:8:6:8:13 | [ensure: raise] call to elements |
3+
| break_ensure.rb:8:6:8:13 | call to elements |
4+
| break_ensure.rb:8:6:8:18 | [ensure: raise] call to nil? |
25
| break_ensure.rb:8:6:8:18 | call to nil? |
3-
| break_ensure.rb:20:10:20:22 | [ensure: break] call to nil? |
4-
| break_ensure.rb:20:10:20:22 | [ensure: raise] call to nil? |
5-
| break_ensure.rb:20:10:20:22 | call to nil? |
6-
| break_ensure.rb:29:8:29:20 | call to nil? |
7-
| break_ensure.rb:35:12:35:12 | [ensure: raise] call to x |
8-
| break_ensure.rb:35:12:35:12 | [ensure: return] call to x |
9-
| break_ensure.rb:35:12:35:12 | call to x |
6+
| break_ensure.rb:20:10:20:15 | [ensure: break] call to nil? |
7+
| break_ensure.rb:20:10:20:15 | [ensure: raise] call to nil? |
8+
| break_ensure.rb:20:10:20:15 | call to nil? |
9+
| break_ensure.rb:29:8:29:13 | call to nil? |
1010
| case.rb:2:8:2:9 | call to x1 |
1111
| case.rb:3:21:3:22 | call to x2 |
1212
| cfg.html.erb:12:24:12:24 | call to a |
@@ -23,6 +23,7 @@ callsWithNoArguments
2323
| cfg.rb:60:17:60:17 | call to b |
2424
| cfg.rb:62:7:62:12 | * ... |
2525
| cfg.rb:62:17:62:27 | * ... |
26+
| cfg.rb:90:1:93:3 | call to each |
2627
| cfg.rb:98:10:98:15 | ** ... |
2728
| cfg.rb:98:30:98:35 | ** ... |
2829
| cfg.rb:138:17:138:23 | * ... |
@@ -56,20 +57,27 @@ callsWithNoArguments
5657
| raise.rb:155:37:155:48 | call to nil? |
5758
| raise.rb:159:3:163:5 | call to foo |
5859
positionalArguments
59-
| break_ensure.rb:3:8:3:18 | ... > ... | break_ensure.rb:3:18:3:18 | 0 |
60+
| break_ensure.rb:2:9:2:13 | ... < ... | break_ensure.rb:2:13:2:13 | 0 |
61+
| break_ensure.rb:3:8:3:12 | ... > ... | break_ensure.rb:3:12:3:12 | 0 |
62+
| break_ensure.rb:9:5:9:23 | [ensure: raise] call to puts | break_ensure.rb:9:10:9:23 | [ensure: raise] "elements nil" |
6063
| break_ensure.rb:9:5:9:23 | call to puts | break_ensure.rb:9:10:9:23 | "elements nil" |
61-
| break_ensure.rb:16:10:16:20 | ... > ... | break_ensure.rb:16:20:16:20 | 0 |
62-
| break_ensure.rb:21:9:21:27 | [ensure: break] call to puts | break_ensure.rb:21:14:21:27 | [ensure: break] "elements nil" |
63-
| break_ensure.rb:21:9:21:27 | [ensure: raise] call to puts | break_ensure.rb:21:14:21:27 | [ensure: raise] "elements nil" |
64-
| break_ensure.rb:21:9:21:27 | call to puts | break_ensure.rb:21:14:21:27 | "elements nil" |
64+
| break_ensure.rb:14:9:14:13 | ... < ... | break_ensure.rb:14:13:14:13 | 0 |
65+
| break_ensure.rb:16:10:16:14 | ... > ... | break_ensure.rb:16:14:16:14 | 0 |
66+
| break_ensure.rb:21:9:21:20 | [ensure: break] call to puts | break_ensure.rb:21:14:21:20 | [ensure: break] "y nil" |
67+
| break_ensure.rb:21:9:21:20 | [ensure: raise] call to puts | break_ensure.rb:21:14:21:20 | [ensure: raise] "y nil" |
68+
| break_ensure.rb:21:9:21:20 | call to puts | break_ensure.rb:21:14:21:20 | "y nil" |
69+
| break_ensure.rb:33:11:33:15 | ... < ... | break_ensure.rb:33:15:33:15 | 0 |
70+
| break_ensure.rb:33:11:33:15 | [ensure: raise] ... < ... | break_ensure.rb:33:15:33:15 | [ensure: raise] 0 |
71+
| break_ensure.rb:33:11:33:15 | [ensure: return] ... < ... | break_ensure.rb:33:15:33:15 | [ensure: return] 0 |
6572
| break_ensure.rb:35:12:35:16 | ... > ... | break_ensure.rb:35:16:35:16 | 0 |
6673
| break_ensure.rb:35:12:35:16 | [ensure: raise] ... > ... | break_ensure.rb:35:16:35:16 | [ensure: raise] 0 |
6774
| break_ensure.rb:35:12:35:16 | [ensure: return] ... > ... | break_ensure.rb:35:16:35:16 | [ensure: return] 0 |
6875
| break_ensure.rb:41:3:41:13 | call to puts | break_ensure.rb:41:8:41:13 | "Done" |
69-
| break_ensure.rb:47:10:47:20 | ... > ... | break_ensure.rb:47:20:47:20 | 1 |
76+
| break_ensure.rb:45:9:45:13 | ... < ... | break_ensure.rb:45:13:45:13 | 0 |
77+
| break_ensure.rb:47:10:47:14 | ... > ... | break_ensure.rb:47:14:47:14 | 1 |
7078
| break_ensure.rb:48:9:48:16 | call to raise | break_ensure.rb:48:15:48:16 | "" |
71-
| break_ensure.rb:51:10:51:20 | ... > ... | break_ensure.rb:51:20:51:20 | 0 |
72-
| break_ensure.rb:51:10:51:20 | [ensure: raise] ... > ... | break_ensure.rb:51:20:51:20 | [ensure: raise] 0 |
79+
| break_ensure.rb:51:10:51:14 | ... > ... | break_ensure.rb:51:14:51:14 | 0 |
80+
| break_ensure.rb:51:10:51:14 | [ensure: raise] ... > ... | break_ensure.rb:51:14:51:14 | [ensure: raise] 0 |
7381
| case.rb:3:29:3:37 | call to puts | case.rb:3:34:3:37 | "x2" |
7482
| case.rb:4:17:4:24 | call to puts | case.rb:4:22:4:24 | "2" |
7583
| cfg.html.erb:6:9:6:58 | call to stylesheet_link_tag | cfg.html.erb:6:29:6:41 | "application" |
@@ -91,7 +99,6 @@ positionalArguments
9199
| cfg.rb:9:1:9:21 | call to [] | cfg.rb:9:14:9:20 | "another" |
92100
| cfg.rb:12:3:12:8 | call to puts | cfg.rb:12:8:12:8 | 4 |
93101
| cfg.rb:16:3:16:14 | call to puts | cfg.rb:16:8:16:14 | "hello" |
94-
| cfg.rb:20:3:20:14 | call to puts | cfg.rb:20:8:20:14 | "world" |
95102
| cfg.rb:23:1:23:6 | ... + ... | cfg.rb:23:6:23:6 | 1 |
96103
| cfg.rb:25:15:25:20 | call to puts | cfg.rb:25:20:25:20 | x |
97104
| cfg.rb:27:1:27:11 | call to puts | cfg.rb:27:6:27:11 | &... |

0 commit comments

Comments
 (0)