Skip to content

Commit 6c71633

Browse files
committed
C#: Skip foreach loop bodies in the CFG when the iteration expression is empty
1 parent 9e240b7 commit 6c71633

File tree

8 files changed

+103
-242
lines changed

8 files changed

+103
-242
lines changed

csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ module ControlFlow {
318318

319319
class BooleanSplit = BooleanSplitting::BooleanSplitImpl;
320320

321-
class LoopUnrollingSplit = LoopUnrollingSplitting::LoopUnrollingSplitImpl;
321+
class LoopSplit = LoopSplitting::LoopSplitImpl;
322322
}
323323

324324
class BasicBlock = BBs::BasicBlock;

csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll

Lines changed: 67 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ private module Cached {
3030
TFinallySplitKind(int nestLevel) { nestLevel = FinallySplitting::nestLevel(_) } or
3131
TExceptionHandlerSplitKind() or
3232
TBooleanSplitKind(BooleanSplitting::BooleanSplitSubKind kind) { kind.startsSplit(_) } or
33-
TLoopUnrollingSplitKind(LoopUnrollingSplitting::UnrollableLoopStmt loop)
33+
TLoopSplitKind(LoopSplitting::AnalyzableLoopStmt loop)
3434

3535
cached
3636
newtype TSplit =
@@ -43,7 +43,7 @@ private module Cached {
4343
kind.startsSplit(_) and
4444
(branch = true or branch = false)
4545
} or
46-
TLoopUnrollingSplit(LoopUnrollingSplitting::UnrollableLoopStmt loop)
46+
TLoopSplit(LoopSplitting::AnalyzableLoopStmt loop)
4747

4848
cached
4949
newtype TSplits =
@@ -1109,7 +1109,7 @@ module BooleanSplitting {
11091109
}
11101110
}
11111111

1112-
module LoopUnrollingSplitting {
1112+
module LoopSplitting {
11131113
private import semmle.code.csharp.controlflow.Guards as Guards
11141114
private import PreBasicBlocks
11151115
private import PreSsa
@@ -1125,51 +1125,78 @@ module LoopUnrollingSplitting {
11251125
}
11261126

11271127
/**
1128-
* A loop where the body is guaranteed to be executed at least once, and
1129-
* can therefore be unrolled in the control flow graph.
1128+
* A loop where the body is guaranteed to be executed at least once, and hence
1129+
* can be unrolled in the control flow graph, or where the body is guaranteed
1130+
* to never be executed, and hence can be removed from the control flow graph.
11301131
*/
1131-
abstract class UnrollableLoopStmt extends LoopStmt {
1132-
/** Holds if the step `pred --c--> succ` should start loop unrolling. */
1133-
abstract predicate startUnroll(ControlFlowElement pred, ControlFlowElement succ, Completion c);
1132+
abstract class AnalyzableLoopStmt extends LoopStmt {
1133+
/** Holds if the step `pred --c--> succ` should start the split. */
1134+
abstract predicate start(ControlFlowElement pred, ControlFlowElement succ, Completion c);
11341135

1135-
/** Holds if the step `pred --c--> succ` should stop loop unrolling. */
1136-
abstract predicate stopUnroll(ControlFlowElement pred, ControlFlowElement succ, Completion c);
1136+
/** Holds if the step `pred --c--> succ` should stop the split. */
1137+
abstract predicate stop(ControlFlowElement pred, ControlFlowElement succ, Completion c);
11371138

11381139
/**
1139-
* Holds if any step `pred --c--> _` should be pruned from the unrolled loop
1140-
* (the loop condition evaluating to `false`).
1140+
* Holds if any step `pred --c--> _` should be pruned from the control flow graph.
11411141
*/
11421142
abstract predicate pruneLoopCondition(ControlFlowElement pred, ConditionalCompletion c);
1143+
1144+
/**
1145+
* Holds if the body is guaranteed to be executed at least once. If not, the
1146+
* body is guaranteed to never be executed.
1147+
*/
1148+
abstract predicate isUnroll();
11431149
}
11441150

1145-
private class UnrollableForeachStmt extends UnrollableLoopStmt, ForeachStmt {
1146-
UnrollableForeachStmt() {
1147-
exists(Guards::AbstractValues::EmptyCollectionValue v | v.isNonEmpty() |
1148-
emptinessGuarded(_, this.getIterableExpr(), v)
1149-
or
1150-
this.getIterableExpr() = v.getAnExpr()
1151-
)
1151+
private class AnalyzableForeachStmt extends AnalyzableLoopStmt, ForeachStmt {
1152+
Guards::AbstractValues::EmptyCollectionValue v;
1153+
1154+
AnalyzableForeachStmt() {
1155+
/*
1156+
* We use `unique` to avoid degenerate cases like
1157+
* ```csharp
1158+
* if (xs.Length == 0)
1159+
* return;
1160+
* if (xs.Length > 0)
1161+
* return;
1162+
* foreach (var x in xs)
1163+
* ....
1164+
* ```
1165+
* where the iterator expression `xs` is guarded by both an emptiness check
1166+
* and a non-emptiness check.
1167+
*/
1168+
1169+
v =
1170+
unique(Guards::AbstractValues::EmptyCollectionValue v0 |
1171+
emptinessGuarded(_, this.getIterableExpr(), v0)
1172+
or
1173+
this.getIterableExpr() = v0.getAnExpr()
1174+
|
1175+
v0
1176+
)
11521177
}
11531178

1154-
override predicate startUnroll(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
1179+
override predicate start(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
11551180
pred = last(this.getIterableExpr(), c) and
11561181
succ = this
11571182
}
11581183

1159-
override predicate stopUnroll(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
1184+
override predicate stop(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
11601185
pred = this and
11611186
succ = succ(pred, c)
11621187
}
11631188

11641189
override predicate pruneLoopCondition(ControlFlowElement pred, ConditionalCompletion c) {
11651190
pred = this and
1166-
c.(EmptinessCompletion).isEmpty()
1191+
c = any(EmptinessCompletion ec | if v.isEmpty() then not ec.isEmpty() else ec.isEmpty())
11671192
}
1193+
1194+
override predicate isUnroll() { v.isNonEmpty() }
11681195
}
11691196

11701197
/**
1171-
* A split for loops where the body is guaranteed to be executed at least once, and
1172-
* can therefore be unrolled in the control flow graph. For example, in
1198+
* A split for loops where the body is guaranteed to be executed at least once, or
1199+
* guaranteed to never be executed. For example, in
11731200
*
11741201
* ```csharp
11751202
* void M(string[] args)
@@ -1184,21 +1211,23 @@ module LoopUnrollingSplitting {
11841211
* the `foreach` loop is guaranteed to be executed at least once, as a result of the
11851212
* `args.Length == 0` check.
11861213
*/
1187-
class LoopUnrollingSplitImpl extends SplitImpl, TLoopUnrollingSplit {
1188-
UnrollableLoopStmt loop;
1214+
class LoopSplitImpl extends SplitImpl, TLoopSplit {
1215+
AnalyzableLoopStmt loop;
11891216

1190-
LoopUnrollingSplitImpl() { this = TLoopUnrollingSplit(loop) }
1217+
LoopSplitImpl() { this = TLoopSplit(loop) }
11911218

11921219
override string toString() {
1193-
result = "unroll (line " + loop.getLocation().getStartLine() + ")"
1220+
if loop.isUnroll()
1221+
then result = "unroll (line " + loop.getLocation().getStartLine() + ")"
1222+
else result = "skip (line " + loop.getLocation().getStartLine() + ")"
11941223
}
11951224
}
11961225

1197-
private int getListOrder(UnrollableLoopStmt loop) {
1226+
private int getListOrder(AnalyzableLoopStmt loop) {
11981227
exists(Callable c, int r | c = loop.getEnclosingCallable() |
11991228
result = r + BooleanSplitting::getNextListOrder() - 1 and
12001229
loop =
1201-
rank[r](UnrollableLoopStmt loop0 |
1230+
rank[r](AnalyzableLoopStmt loop0 |
12021231
loop0.getEnclosingCallable() = c
12031232
|
12041233
loop0 order by loop0.getLocation().getStartLine(), loop0.getLocation().getStartColumn()
@@ -1210,21 +1239,21 @@ module LoopUnrollingSplitting {
12101239
result = max(int i | i = getListOrder(_) + 1 or i = BooleanSplitting::getNextListOrder())
12111240
}
12121241

1213-
private class LoopUnrollingSplitKind extends SplitKind, TLoopUnrollingSplitKind {
1214-
private UnrollableLoopStmt loop;
1242+
private class LoopSplitKind extends SplitKind, TLoopSplitKind {
1243+
private AnalyzableLoopStmt loop;
12151244

1216-
LoopUnrollingSplitKind() { this = TLoopUnrollingSplitKind(loop) }
1245+
LoopSplitKind() { this = TLoopSplitKind(loop) }
12171246

12181247
override int getListOrder() { result = getListOrder(loop) }
12191248

12201249
override string toString() { result = "Unroll" }
12211250
}
12221251

1223-
private class LoopUnrollingSplitInternal extends SplitInternal, LoopUnrollingSplitImpl {
1224-
override LoopUnrollingSplitKind getKind() { result = TLoopUnrollingSplitKind(loop) }
1252+
private class LoopUnrollingSplitInternal extends SplitInternal, LoopSplitImpl {
1253+
override LoopSplitKind getKind() { result = TLoopSplitKind(loop) }
12251254

12261255
override predicate hasEntry(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
1227-
loop.startUnroll(pred, succ, c)
1256+
loop.start(pred, succ, c)
12281257
}
12291258

12301259
override predicate hasEntry(Callable pred, ControlFlowElement succ) { none() }
@@ -1241,7 +1270,7 @@ module LoopUnrollingSplitting {
12411270

12421271
override predicate hasExit(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
12431272
this.appliesToPredecessor(pred, c) and
1244-
loop.stopUnroll(pred, succ, c)
1273+
loop.stop(pred, succ, c)
12451274
}
12461275

12471276
override Callable hasExit(ControlFlowElement pred, Completion c) {
@@ -1252,7 +1281,7 @@ module LoopUnrollingSplitting {
12521281
override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
12531282
this.appliesToPredecessor(pred, c) and
12541283
succ = succ(pred, c) and
1255-
not loop.stopUnroll(pred, succ, c)
1284+
not loop.stop(pred, succ, c)
12561285
}
12571286
}
12581287
}

csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -429,10 +429,7 @@
429429
| LoopUnrolling.cs:24:9:26:40 | foreach (... ... in ...) ... | LoopUnrolling.cs:24:9:26:40 | foreach (... ... in ...) ... | 1 |
430430
| LoopUnrolling.cs:24:22:24:24 | Char arg | LoopUnrolling.cs:25:13:26:40 | [unroll (line 25)] foreach (... ... in ...) ... | 3 |
431431
| LoopUnrolling.cs:25:26:25:29 | Char arg0 | LoopUnrolling.cs:25:13:26:40 | foreach (... ... in ...) ... | 5 |
432-
| LoopUnrolling.cs:29:10:29:11 | enter M4 | LoopUnrolling.cs:32:27:32:28 | access to local variable xs | 7 |
433-
| LoopUnrolling.cs:29:10:29:11 | exit M4 | LoopUnrolling.cs:29:10:29:11 | exit M4 | 1 |
434-
| LoopUnrolling.cs:32:9:33:33 | foreach (... ... in ...) ... | LoopUnrolling.cs:32:9:33:33 | foreach (... ... in ...) ... | 1 |
435-
| LoopUnrolling.cs:32:22:32:22 | String x | LoopUnrolling.cs:33:13:33:32 | call to method WriteLine | 4 |
432+
| LoopUnrolling.cs:29:10:29:11 | enter M4 | LoopUnrolling.cs:29:10:29:11 | exit M4 | 9 |
436433
| LoopUnrolling.cs:36:10:36:11 | enter M5 | LoopUnrolling.cs:40:9:42:41 | [unroll (line 40)] foreach (... ... in ...) ... | 18 |
437434
| LoopUnrolling.cs:36:10:36:11 | exit M5 | LoopUnrolling.cs:36:10:36:11 | exit M5 | 1 |
438435
| LoopUnrolling.cs:40:9:42:41 | foreach (... ... in ...) ... | LoopUnrolling.cs:40:9:42:41 | foreach (... ... in ...) ... | 1 |
@@ -449,17 +446,9 @@
449446
| LoopUnrolling.cs:67:10:67:11 | enter M8 | LoopUnrolling.cs:69:14:69:23 | call to method Any | 6 |
450447
| LoopUnrolling.cs:67:10:67:11 | exit M8 | LoopUnrolling.cs:67:10:67:11 | exit M8 | 1 |
451448
| LoopUnrolling.cs:70:13:70:19 | return ...; | LoopUnrolling.cs:70:13:70:19 | return ...; | 1 |
452-
| LoopUnrolling.cs:71:9:71:21 | ...; | LoopUnrolling.cs:72:29:72:32 | access to parameter args | 4 |
453-
| LoopUnrolling.cs:72:9:73:35 | foreach (... ... in ...) ... | LoopUnrolling.cs:72:9:73:35 | foreach (... ... in ...) ... | 1 |
454-
| LoopUnrolling.cs:72:22:72:24 | String arg | LoopUnrolling.cs:73:13:73:34 | call to method WriteLine | 4 |
455-
| LoopUnrolling.cs:76:10:76:11 | enter M9 | LoopUnrolling.cs:79:27:79:28 | access to local variable xs | 8 |
456-
| LoopUnrolling.cs:76:10:76:11 | exit M9 | LoopUnrolling.cs:76:10:76:11 | exit M9 | 1 |
457-
| LoopUnrolling.cs:79:9:82:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:79:9:82:9 | foreach (... ... in ...) ... | 1 |
458-
| LoopUnrolling.cs:79:22:79:22 | String x | LoopUnrolling.cs:81:13:81:32 | call to method WriteLine | 5 |
459-
| LoopUnrolling.cs:85:10:85:12 | enter M10 | LoopUnrolling.cs:88:27:88:28 | access to local variable xs | 8 |
460-
| LoopUnrolling.cs:85:10:85:12 | exit M10 | LoopUnrolling.cs:85:10:85:12 | exit M10 | 1 |
461-
| LoopUnrolling.cs:88:9:91:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:88:9:91:9 | foreach (... ... in ...) ... | 1 |
462-
| LoopUnrolling.cs:88:22:88:22 | String x | LoopUnrolling.cs:90:13:90:32 | call to method WriteLine | 5 |
449+
| LoopUnrolling.cs:71:9:71:21 | ...; | LoopUnrolling.cs:72:9:73:35 | [skip (line 72)] foreach (... ... in ...) ... | 5 |
450+
| LoopUnrolling.cs:76:10:76:11 | enter M9 | LoopUnrolling.cs:76:10:76:11 | exit M9 | 10 |
451+
| LoopUnrolling.cs:85:10:85:12 | enter M10 | LoopUnrolling.cs:85:10:85:12 | exit M10 | 10 |
463452
| LoopUnrolling.cs:94:10:94:12 | enter M11 | LoopUnrolling.cs:97:9:100:9 | [unroll (line 97)] foreach (... ... in ...) ... | 9 |
464453
| LoopUnrolling.cs:94:10:94:12 | exit M11 | LoopUnrolling.cs:94:10:94:12 | exit M11 | 1 |
465454
| LoopUnrolling.cs:97:22:97:22 | String x | LoopUnrolling.cs:97:9:100:9 | foreach (... ... in ...) ... | 6 |

csharp/ql/test/library-tests/controlflow/graph/Condition.expected

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -434,8 +434,6 @@ conditionBlock
434434
| LoopUnrolling.cs:24:9:26:40 | foreach (... ... in ...) ... | LoopUnrolling.cs:24:22:24:24 | Char arg | false |
435435
| LoopUnrolling.cs:24:9:26:40 | foreach (... ... in ...) ... | LoopUnrolling.cs:25:26:25:29 | Char arg0 | false |
436436
| LoopUnrolling.cs:24:22:24:24 | Char arg | LoopUnrolling.cs:25:26:25:29 | Char arg0 | false |
437-
| LoopUnrolling.cs:32:9:33:33 | foreach (... ... in ...) ... | LoopUnrolling.cs:29:10:29:11 | exit M4 | true |
438-
| LoopUnrolling.cs:32:9:33:33 | foreach (... ... in ...) ... | LoopUnrolling.cs:32:22:32:22 | String x | false |
439437
| LoopUnrolling.cs:36:10:36:11 | enter M5 | LoopUnrolling.cs:36:10:36:11 | exit M5 | false |
440438
| LoopUnrolling.cs:36:10:36:11 | enter M5 | LoopUnrolling.cs:40:9:42:41 | foreach (... ... in ...) ... | false |
441439
| LoopUnrolling.cs:36:10:36:11 | enter M5 | LoopUnrolling.cs:40:22:40:22 | String x | false |
@@ -454,13 +452,6 @@ conditionBlock
454452
| LoopUnrolling.cs:62:13:63:37 | [b (line 55): false] if (...) ... | LoopUnrolling.cs:58:22:58:22 | [b (line 55): false] String x | false |
455453
| LoopUnrolling.cs:67:10:67:11 | enter M8 | LoopUnrolling.cs:70:13:70:19 | return ...; | false |
456454
| LoopUnrolling.cs:67:10:67:11 | enter M8 | LoopUnrolling.cs:71:9:71:21 | ...; | true |
457-
| LoopUnrolling.cs:67:10:67:11 | enter M8 | LoopUnrolling.cs:72:9:73:35 | foreach (... ... in ...) ... | true |
458-
| LoopUnrolling.cs:67:10:67:11 | enter M8 | LoopUnrolling.cs:72:22:72:24 | String arg | true |
459-
| LoopUnrolling.cs:72:9:73:35 | foreach (... ... in ...) ... | LoopUnrolling.cs:72:22:72:24 | String arg | false |
460-
| LoopUnrolling.cs:79:9:82:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:76:10:76:11 | exit M9 | true |
461-
| LoopUnrolling.cs:79:9:82:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:79:22:79:22 | String x | false |
462-
| LoopUnrolling.cs:88:9:91:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:85:10:85:12 | exit M10 | true |
463-
| LoopUnrolling.cs:88:9:91:9 | foreach (... ... in ...) ... | LoopUnrolling.cs:88:22:88:22 | String x | false |
464455
| LoopUnrolling.cs:94:10:94:12 | enter M11 | LoopUnrolling.cs:94:10:94:12 | exit M11 | false |
465456
| LoopUnrolling.cs:94:10:94:12 | enter M11 | LoopUnrolling.cs:97:22:97:22 | String x | false |
466457
| LoopUnrolling.cs:97:22:97:22 | String x | LoopUnrolling.cs:94:10:94:12 | exit M11 | true |

0 commit comments

Comments
 (0)