Skip to content

Commit 5f77ac4

Browse files
author
Robert Marsh
authored
Merge pull request #1325 from jbj/reachableRecursive
C++: reachableRecursive refactor for performance
2 parents 65cbd47 + 947aaa9 commit 5f77ac4

File tree

2 files changed

+116
-101
lines changed

2 files changed

+116
-101
lines changed

‎cpp/ql/src/semmle/code/cpp/controlflow/ControlFlowGraph.qll‎

Lines changed: 1 addition & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -75,85 +75,7 @@ class ControlFlowNode extends Locatable, ControlFlowNodeBase {
7575
}
7676
}
7777

78-
import Cached
79-
private cached module Cached {
80-
// The base case of `reachable` is factored out for performance. If it's
81-
// written in-line in `reachable`, the compiler inserts a `n instanceof
82-
// ControlFlowNode` check because the `not ... and not ...` case doesn't
83-
// otherwise bind `n`. The problem is that this check is inserted at the
84-
// outermost level of this predicate, so it covers all cases including the
85-
// recursive case. The optimizer doesn't eliminate the check even though it's
86-
// redundant, and having the check leads to needless extra computation and a
87-
// risk of bad join orders.
88-
private predicate reachableBaseCase(ControlFlowNode n) {
89-
exists(Function f | f.getEntryPoint() = n)
90-
or
91-
// Okay to use successors_extended directly here
92-
(not successors_extended(_,n) and not successors_extended(n,_))
93-
or
94-
n instanceof Handler
95-
}
96-
97-
/**
98-
* Holds if the control-flow node `n` is reachable, meaning that either
99-
* it is an entry point, or there exists a path in the control-flow
100-
* graph of its function that connects an entry point to it.
101-
* Compile-time constant conditions are taken into account, so that
102-
* the call to `f` is not reachable in `if (0) f();` even if the
103-
* `if` statement as a whole is reachable.
104-
*/
105-
cached
106-
predicate reachable(ControlFlowNode n)
107-
{
108-
reachableBaseCase(n)
109-
or
110-
reachable(n.getAPredecessor())
111-
}
112-
113-
/** Holds if `condition` always evaluates to a nonzero value. */
114-
cached
115-
predicate conditionAlwaysTrue(Expr condition) {
116-
conditionAlways(condition, true)
117-
}
118-
119-
/** Holds if `condition` always evaluates to zero. */
120-
cached
121-
predicate conditionAlwaysFalse(Expr condition) {
122-
conditionAlways(condition, false)
123-
or
124-
// If a loop condition evaluates to false upon entry, it will always
125-
// be false
126-
loopConditionAlwaysUponEntry(_, condition, false)
127-
}
128-
129-
/**
130-
* The condition `condition` for the loop `loop` is provably `true` upon entry.
131-
* That is, at least one iteration of the loop is guaranteed.
132-
*/
133-
cached
134-
predicate loopConditionAlwaysTrueUponEntry(ControlFlowNode loop, Expr condition) {
135-
loopConditionAlwaysUponEntry(loop, condition, true)
136-
}
137-
}
138-
139-
private predicate conditionAlways(Expr condition, boolean b) {
140-
exists(ConditionEvaluator x, int val |
141-
val = x.getValue(condition) |
142-
val != 0 and b = true
143-
or
144-
val = 0 and b = false
145-
)
146-
}
147-
148-
private predicate loopConditionAlwaysUponEntry(ControlFlowNode loop, Expr condition, boolean b) {
149-
exists(LoopEntryConditionEvaluator x, int val |
150-
x.isLoopEntry(condition, loop) and
151-
val = x.getValue(condition) |
152-
val != 0 and b = true
153-
or
154-
val = 0 and b = false
155-
)
156-
}
78+
import ControlFlowGraphPublic
15779

15880
/**
15981
* An element that is convertible to `ControlFlowNode`. This class is similar

‎cpp/ql/src/semmle/code/cpp/controlflow/internal/ConstantExprs.qll‎

Lines changed: 115 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,110 @@
11
import cpp
22
private import PrimitiveBasicBlocks
33
private class Node = ControlFlowNodeBase;
4+
import Cached
5+
6+
cached
7+
private module Cached {
8+
/** A call to a function known not to return. */
9+
cached
10+
predicate aborting(FunctionCall c) {
11+
not potentiallyReturningFunctionCall(c)
12+
}
13+
14+
/**
15+
* Functions that are known not to return. This is normally because the function
16+
* exits the program or longjmps to another location.
17+
*/
18+
cached
19+
predicate abortingFunction(Function f) {
20+
not potentiallyReturningFunction(f)
21+
}
22+
23+
/**
24+
* An adapted version of the `successors_extended` relation that excludes
25+
* impossible control-flow edges - flow will never occur along these
26+
* edges, so it is safe (and indeed sensible) to remove them.
27+
*/
28+
cached predicate successors_adapted(Node pred, Node succ) {
29+
successors_extended(pred, succ)
30+
and possiblePredecessor(pred)
31+
and not impossibleFalseEdge(pred, succ)
32+
and not impossibleTrueEdge(pred, succ)
33+
and not impossibleSwitchEdge(pred, succ)
34+
and not impossibleDefaultSwitchEdge(pred, succ)
35+
and not impossibleFunctionReturn(pred, succ)
36+
and not getOptions().exprExits(pred)
37+
}
438

5-
/** A call to a function known not to return. */
6-
predicate aborting(FunctionCall c) {
7-
not potentiallyReturningFunctionCall(c)
39+
/**
40+
* Provides predicates that should be exported as if they were top-level
41+
* predicates in `ControlFlowGraph.qll`. They have to be defined in this file
42+
* in order to be grouped in a `cached module` with other predicates that
43+
* must go in the same cached stage.
44+
*/
45+
cached
46+
module ControlFlowGraphPublic {
47+
/**
48+
* Holds if the control-flow node `n` is reachable, meaning that either
49+
* it is an entry point, or there exists a path in the control-flow
50+
* graph of its function that connects an entry point to it.
51+
* Compile-time constant conditions are taken into account, so that
52+
* the call to `f` is not reachable in `if (0) f();` even if the
53+
* `if` statement as a whole is reachable.
54+
*/
55+
cached
56+
predicate reachable(ControlFlowNode n)
57+
{
58+
// Okay to use successors_extended directly here
59+
reachableRecursive(n)
60+
or
61+
(not successors_extended(_,n) and not successors_extended(n,_))
62+
}
63+
64+
/** Holds if `condition` always evaluates to a nonzero value. */
65+
cached
66+
predicate conditionAlwaysTrue(Expr condition) {
67+
conditionAlways(condition, true)
68+
}
69+
70+
/** Holds if `condition` always evaluates to zero. */
71+
cached
72+
predicate conditionAlwaysFalse(Expr condition) {
73+
conditionAlways(condition, false)
74+
or
75+
// If a loop condition evaluates to false upon entry, it will always
76+
// be false
77+
loopConditionAlwaysUponEntry(_, condition, false)
78+
}
79+
80+
/**
81+
* The condition `condition` for the loop `loop` is provably `true` upon entry.
82+
* That is, at least one iteration of the loop is guaranteed.
83+
*/
84+
cached
85+
predicate loopConditionAlwaysTrueUponEntry(ControlFlowNode loop, Expr condition) {
86+
loopConditionAlwaysUponEntry(loop, condition, true)
87+
}
88+
}
889
}
990

10-
/**
11-
* Functions that are known not to return. This is normally because the function
12-
* exits the program or longjmps to another location.
13-
*/
14-
predicate abortingFunction(Function f) {
15-
not potentiallyReturningFunction(f)
91+
private predicate conditionAlways(Expr condition, boolean b) {
92+
exists(ConditionEvaluator x, int val |
93+
val = x.getValue(condition) |
94+
val != 0 and b = true
95+
or
96+
val = 0 and b = false
97+
)
98+
}
99+
100+
private predicate loopConditionAlwaysUponEntry(ControlFlowNode loop, Expr condition, boolean b) {
101+
exists(LoopEntryConditionEvaluator x, int val |
102+
x.isLoopEntry(condition, loop) and
103+
val = x.getValue(condition) |
104+
val != 0 and b = true
105+
or
106+
val = 0 and b = false
107+
)
16108
}
17109

18110
/**
@@ -61,7 +153,7 @@ private predicate potentiallyReturningFunction(Function f) {
61153
nonAnalyzableFunction(f)
62154
or
63155
// otherwise the exit-point of `f` must be reachable
64-
reachable(f)
156+
reachableRecursive(f)
65157
)
66158
}
67159

@@ -202,19 +294,20 @@ private predicate possiblePredecessor(Node pred) {
202294
}
203295

204296
/**
205-
* An adapted version of the `successors_extended` relation that excludes
206-
* impossible control-flow edges - flow will never occur along these
207-
* edges, so it is safe (and indeed sensible) to remove them.
297+
* Holds if the control-flow node `n` is reachable, meaning that either
298+
* it is an entry point, or there exists a path in the control-flow
299+
* graph of its function that connects an entry point to it.
300+
* Compile-time constant conditions are taken into account, so that
301+
* the call to `f` is not reachable in `if (0) f();` even if the
302+
* `if` statement as a whole is reachable.
208303
*/
209-
cached predicate successors_adapted(Node pred, Node succ) {
210-
successors_extended(pred, succ)
211-
and possiblePredecessor(pred)
212-
and not impossibleFalseEdge(pred, succ)
213-
and not impossibleTrueEdge(pred, succ)
214-
and not impossibleSwitchEdge(pred, succ)
215-
and not impossibleDefaultSwitchEdge(pred, succ)
216-
and not impossibleFunctionReturn(pred, succ)
217-
and not getOptions().exprExits(pred)
304+
private predicate reachableRecursive(ControlFlowNode n)
305+
{
306+
exists(Function f | f.getEntryPoint() = n)
307+
or
308+
n instanceof Handler
309+
or
310+
reachableRecursive(n.getAPredecessor())
218311
}
219312

220313
private predicate compileTimeConstantInt(Expr e, int val) {

0 commit comments

Comments
 (0)