Skip to content

Commit 28261d6

Browse files
geoffw0geoffw0
authored
Merge pull request #737 from jbj/cfg-perf
C++: QL CFG performance and tweaks
2 parents f474fdd + 9146b8e commit 28261d6

File tree

3 files changed

+80
-12
lines changed

3 files changed

+80
-12
lines changed

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

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -827,7 +827,8 @@ private predicate straightLineDense(Node scope, int rnk, Node nrnk, Spec spec) {
827827
* but most cases should be handled through one of the convenience predicates
828828
* as outlined in the comment at the top of this file.
829829
*/
830-
private predicate subEdge(Node n1, Pos p1, Node n2, Pos p2) {
830+
// The parameters are ordered this way for performance.
831+
private predicate subEdge(Pos p1, Node n1, Node n2, Pos p2) {
831832
exists(Node scope, int rnk, Spec spec1, Spec spec2 |
832833
straightLineDense(scope, rnk, n1, spec1) and
833834
straightLineDense(scope, rnk + 1, n2, spec2) and
@@ -997,13 +998,13 @@ private predicate subEdge(Node n1, Pos p1, Node n2, Pos p2) {
997998
* predicate includes all sub-edges except those with true/false labels (see
998999
* `conditionJumps`).
9991000
*/
1000-
private predicate subEdgeIncludingDestructors(Node n1, Pos p1, Node n2, Pos p2) {
1001-
subEdge(n1, p1, n2, p2)
1001+
private predicate subEdgeIncludingDestructors(Pos p1, Node n1, Node n2, Pos p2) {
1002+
subEdge(p1, n1, n2, p2)
10021003
or
10031004
// If `n1` has sub-nodes to accomodate destructors, but there are none to be
10041005
// called, connect the "before destructors" node directly to the "after
10051006
// destructors" node. For performance, only do this when the nodes exist.
1006-
exists(Pos afterDtors | afterDtors.isAfterDestructors() | subEdge(n1, afterDtors, _, _)) and
1007+
exists(Pos afterDtors | afterDtors.isAfterDestructors() | subEdge(afterDtors, n1, _, _)) and
10071008
not exists(getDestructorCallAfterNode(n1, 0)) and
10081009
p1.nodeBeforeDestructors(n1, n1) and
10091010
p2.nodeAfterDestructors(n2, n1)
@@ -1286,22 +1287,27 @@ private predicate conditionJumps(Expr test, boolean truth, Node n2, Pos p2) {
12861287
)
12871288
}
12881289

1290+
// Factored out for performance. See QL-796.
1291+
private predicate normalGroupMemberBaseCase(Node memberNode, Pos memberPos, Node atNode) {
1292+
memberNode = atNode and
1293+
memberPos.isAt() and
1294+
// We check for excludeNode here as it's slower to check in all the leaf
1295+
// cases during construction of the sub-graph.
1296+
not excludeNode(atNode)
1297+
}
1298+
12891299
/**
12901300
* Holds if the sub-node `(memberNode, memberPos)` can reach `at(atNode)` by
12911301
* following sub-edges forward without crossing another "at" node. Here,
12921302
* `memberPos.isAt()` holds only when `memberNode = atNode`.
12931303
*/
12941304
private predicate normalGroupMember(Node memberNode, Pos memberPos, Node atNode) {
1295-
memberNode = atNode and
1296-
memberPos.isAt() and
1297-
// We check for excludeNode here as it's slower to check in all the leaf
1298-
// cases during construction of the sub-graph.
1299-
not excludeNode(atNode)
1305+
normalGroupMemberBaseCase(memberNode, memberPos, atNode)
13001306
or
13011307
exists(Node succNode, Pos succPos |
13021308
normalGroupMember(succNode, succPos, atNode) and
13031309
not memberPos.isAt() and
1304-
subEdgeIncludingDestructors(memberNode, memberPos, succNode, succPos)
1310+
subEdgeIncludingDestructors(memberPos, memberNode, succNode, succPos)
13051311
)
13061312
}
13071313

@@ -1317,7 +1323,7 @@ private predicate precedesCondition(Node memberNode, Pos memberPos, Node test) {
13171323
or
13181324
exists(Node succNode, Pos succPos |
13191325
precedesCondition(succNode, succPos, test) and
1320-
subEdgeIncludingDestructors(memberNode, memberPos, succNode, succPos) and
1326+
subEdgeIncludingDestructors(memberPos, memberNode, succNode, succPos) and
13211327
// Unlike the similar TC in normalGroupMember we're here including the
13221328
// At-node in the group. This should generalize better to the case where
13231329
// the base case isn't always an After-node.
@@ -1355,7 +1361,7 @@ private module Cached {
13551361
cached
13561362
predicate qlCFGSuccessor(Node n1, Node n2) {
13571363
exists(Node memberNode, Pos memberPos |
1358-
subEdgeIncludingDestructors(n1, any(Pos at | at.isAt()), memberNode, memberPos) and
1364+
subEdgeIncludingDestructors(any(Pos at | at.isAt()), n1, memberNode, memberPos) and
13591365
normalGroupMember(memberNode, memberPos, n2)
13601366
)
13611367
or

‎cpp/ql/test/library-tests/qlcfg/cfg.expected‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6084,6 +6084,48 @@
60846084
| caller | true | 21161 | 21159 | |
60856085
| caller | true | 21163 | 21116 | |
60866086
| caller | true | 21165 | 21126 | |
6087+
| cond_destruct::C::C | false | 4133 | 4133 | C |
6088+
| cond_destruct::C::C | false | 4173 | 4173 | C |
6089+
| cond_destruct::C::getInt | false | 4119 | 4119 | getInt |
6090+
| cond_destruct::C::operator= | false | 4167 | 4167 | operator= |
6091+
| cond_destruct::C::~C | false | 4163 | 4163 | ~C |
6092+
| cond_destruct::f | false | 4107 | 4107 | f |
6093+
| cond_destruct::f | false | 4115 | 4115 | declaration |
6094+
| cond_destruct::f | false | 4117 | 4117 | declaration |
6095+
| cond_destruct::f | false | 4122 | 4122 | call to getInt |
6096+
| cond_destruct::f | false | 4128 | 4128 | x |
6097+
| cond_destruct::f | false | 4130 | 4130 | (bool)... |
6098+
| cond_destruct::f | false | 4131 | 4131 | call to C |
6099+
| cond_destruct::f | false | 4134 | 4134 | (const C)... |
6100+
| cond_destruct::f | false | 4137 | 4137 | call to C |
6101+
| cond_destruct::f | false | 4139 | 4139 | initializer for local |
6102+
| cond_destruct::f | false | 4142 | 4142 | local |
6103+
| cond_destruct::f | false | 4144 | 4144 | (const C)... |
6104+
| cond_destruct::f | false | 4146 | 4146 | ... ? ... : ... |
6105+
| cond_destruct::f | false | 4148 | 4148 | (reference to) |
6106+
| cond_destruct::f | false | 4149 | 4149 | initializer for ref |
6107+
| cond_destruct::f | false | 4153 | 4153 | ref |
6108+
| cond_destruct::f | false | 4155 | 4155 | (reference dereference) |
6109+
| cond_destruct::f | false | 4156 | 4156 | return ... |
6110+
| cond_destruct::f | false | 4158 | 4158 | { ... } |
6111+
| cond_destruct::f | false | 4160 | 4160 | local |
6112+
| cond_destruct::f | false | 4162 | 4162 | call to local.~C |
6113+
| cond_destruct::f | true | 4115 | 4139 | |
6114+
| cond_destruct::f | true | 4117 | 4149 | |
6115+
| cond_destruct::f | true | 4122 | 4160 | |
6116+
| cond_destruct::f | true | 4128 | 4131 | T |
6117+
| cond_destruct::f | true | 4128 | 4142 | F |
6118+
| cond_destruct::f | true | 4131 | 4156 | |
6119+
| cond_destruct::f | true | 4137 | 4117 | |
6120+
| cond_destruct::f | true | 4139 | 4137 | |
6121+
| cond_destruct::f | true | 4142 | 4156 | |
6122+
| cond_destruct::f | true | 4146 | 4128 | |
6123+
| cond_destruct::f | true | 4149 | 4146 | |
6124+
| cond_destruct::f | true | 4153 | 4122 | |
6125+
| cond_destruct::f | true | 4156 | 4153 | |
6126+
| cond_destruct::f | true | 4158 | 4115 | |
6127+
| cond_destruct::f | true | 4160 | 4162 | |
6128+
| cond_destruct::f | true | 4162 | 4107 | |
60876129
| cpp_fun | false | 22196 | 22196 | cpp_fun |
60886130
| cpp_fun | false | 22201 | 22201 | declaration |
60896131
| cpp_fun | false | 22203 | 22203 | declaration |

‎cpp/ql/test/library-tests/qlcfg/destructors.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,23 @@ void destructor_catch() {
3636
HasDtor d2 = { 0 };
3737
}
3838
}
39+
40+
namespace cond_destruct {
41+
struct C {
42+
C();
43+
C(const C&) = delete;
44+
~C();
45+
int getInt() const;
46+
void *data;
47+
};
48+
49+
int f(int x) {
50+
C local;
51+
const C &ref = x ? (const C&)C() : (const C&)local;
52+
return ref.getInt();
53+
// If `x` was true, `ref` refers to a temporary object whose lifetime was
54+
// extended to coincide with `ref`. Before the function returns, it
55+
// should destruct `ref` if and only if the first branch was taken in the
56+
// ?: expression.
57+
}
58+
}

0 commit comments

Comments
 (0)