Skip to content

C++: QL CFG performance and tweaks #737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 10, 2019
Merged

C++: QL CFG performance and tweaks #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c74b891
C++: Reorder parameters to subEdge relations
jbj Jan 8, 2019
8ac826a
C++: Factor out base case of normalGroupMember
jbj Jan 8, 2019
9146b8e
C++: Add example of conditional destruction
jbj Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions cpp/ql/src/semmle/code/cpp/controlflow/internal/CFG.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,8 @@ private predicate straightLineDense(Node scope, int rnk, Node nrnk, Spec spec) {
* but most cases should be handled through one of the convenience predicates
* as outlined in the comment at the top of this file.
*/
private predicate subEdge(Node n1, Pos p1, Node n2, Pos p2) {
// The parameters are ordered this way for performance.
private predicate subEdge(Pos p1, Node n1, Node n2, Pos p2) {
exists(Node scope, int rnk, Spec spec1, Spec spec2 |
straightLineDense(scope, rnk, n1, spec1) and
straightLineDense(scope, rnk + 1, n2, spec2) and
Expand Down Expand Up @@ -997,13 +998,13 @@ private predicate subEdge(Node n1, Pos p1, Node n2, Pos p2) {
* predicate includes all sub-edges except those with true/false labels (see
* `conditionJumps`).
*/
private predicate subEdgeIncludingDestructors(Node n1, Pos p1, Node n2, Pos p2) {
subEdge(n1, p1, n2, p2)
private predicate subEdgeIncludingDestructors(Pos p1, Node n1, Node n2, Pos p2) {
subEdge(p1, n1, n2, p2)
or
// If `n1` has sub-nodes to accomodate destructors, but there are none to be
// called, connect the "before destructors" node directly to the "after
// destructors" node. For performance, only do this when the nodes exist.
exists(Pos afterDtors | afterDtors.isAfterDestructors() | subEdge(n1, afterDtors, _, _)) and
exists(Pos afterDtors | afterDtors.isAfterDestructors() | subEdge(afterDtors, n1, _, _)) and
not exists(getDestructorCallAfterNode(n1, 0)) and
p1.nodeBeforeDestructors(n1, n1) and
p2.nodeAfterDestructors(n2, n1)
Expand Down Expand Up @@ -1286,22 +1287,27 @@ private predicate conditionJumps(Expr test, boolean truth, Node n2, Pos p2) {
)
}

// Factored out for performance. See QL-796.
private predicate normalGroupMemberBaseCase(Node memberNode, Pos memberPos, Node atNode) {
memberNode = atNode and
memberPos.isAt() and
// We check for excludeNode here as it's slower to check in all the leaf
// cases during construction of the sub-graph.
not excludeNode(atNode)
}

/**
* Holds if the sub-node `(memberNode, memberPos)` can reach `at(atNode)` by
* following sub-edges forward without crossing another "at" node. Here,
* `memberPos.isAt()` holds only when `memberNode = atNode`.
*/
private predicate normalGroupMember(Node memberNode, Pos memberPos, Node atNode) {
memberNode = atNode and
memberPos.isAt() and
// We check for excludeNode here as it's slower to check in all the leaf
// cases during construction of the sub-graph.
not excludeNode(atNode)
normalGroupMemberBaseCase(memberNode, memberPos, atNode)
or
exists(Node succNode, Pos succPos |
normalGroupMember(succNode, succPos, atNode) and
not memberPos.isAt() and
subEdgeIncludingDestructors(memberNode, memberPos, succNode, succPos)
subEdgeIncludingDestructors(memberPos, memberNode, succNode, succPos)
)
}

Expand All @@ -1317,7 +1323,7 @@ private predicate precedesCondition(Node memberNode, Pos memberPos, Node test) {
or
exists(Node succNode, Pos succPos |
precedesCondition(succNode, succPos, test) and
subEdgeIncludingDestructors(memberNode, memberPos, succNode, succPos) and
subEdgeIncludingDestructors(memberPos, memberNode, succNode, succPos) and
// Unlike the similar TC in normalGroupMember we're here including the
// At-node in the group. This should generalize better to the case where
// the base case isn't always an After-node.
Expand Down Expand Up @@ -1355,7 +1361,7 @@ private module Cached {
cached
predicate qlCFGSuccessor(Node n1, Node n2) {
exists(Node memberNode, Pos memberPos |
subEdgeIncludingDestructors(n1, any(Pos at | at.isAt()), memberNode, memberPos) and
subEdgeIncludingDestructors(any(Pos at | at.isAt()), n1, memberNode, memberPos) and
normalGroupMember(memberNode, memberPos, n2)
)
or
Expand Down
42 changes: 42 additions & 0 deletions cpp/ql/test/library-tests/qlcfg/cfg.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6084,6 +6084,48 @@
| caller | true | 21161 | 21159 | |
| caller | true | 21163 | 21116 | |
| caller | true | 21165 | 21126 | |
| cond_destruct::C::C | false | 4133 | 4133 | C |
| cond_destruct::C::C | false | 4173 | 4173 | C |
| cond_destruct::C::getInt | false | 4119 | 4119 | getInt |
| cond_destruct::C::operator= | false | 4167 | 4167 | operator= |
| cond_destruct::C::~C | false | 4163 | 4163 | ~C |
| cond_destruct::f | false | 4107 | 4107 | f |
| cond_destruct::f | false | 4115 | 4115 | declaration |
| cond_destruct::f | false | 4117 | 4117 | declaration |
| cond_destruct::f | false | 4122 | 4122 | call to getInt |
| cond_destruct::f | false | 4128 | 4128 | x |
| cond_destruct::f | false | 4130 | 4130 | (bool)... |
| cond_destruct::f | false | 4131 | 4131 | call to C |
| cond_destruct::f | false | 4134 | 4134 | (const C)... |
| cond_destruct::f | false | 4137 | 4137 | call to C |
| cond_destruct::f | false | 4139 | 4139 | initializer for local |
| cond_destruct::f | false | 4142 | 4142 | local |
| cond_destruct::f | false | 4144 | 4144 | (const C)... |
| cond_destruct::f | false | 4146 | 4146 | ... ? ... : ... |
| cond_destruct::f | false | 4148 | 4148 | (reference to) |
| cond_destruct::f | false | 4149 | 4149 | initializer for ref |
| cond_destruct::f | false | 4153 | 4153 | ref |
| cond_destruct::f | false | 4155 | 4155 | (reference dereference) |
| cond_destruct::f | false | 4156 | 4156 | return ... |
| cond_destruct::f | false | 4158 | 4158 | { ... } |
| cond_destruct::f | false | 4160 | 4160 | local |
| cond_destruct::f | false | 4162 | 4162 | call to local.~C |
| cond_destruct::f | true | 4115 | 4139 | |
| cond_destruct::f | true | 4117 | 4149 | |
| cond_destruct::f | true | 4122 | 4160 | |
| cond_destruct::f | true | 4128 | 4131 | T |
| cond_destruct::f | true | 4128 | 4142 | F |
| cond_destruct::f | true | 4131 | 4156 | |
| cond_destruct::f | true | 4137 | 4117 | |
| cond_destruct::f | true | 4139 | 4137 | |
| cond_destruct::f | true | 4142 | 4156 | |
| cond_destruct::f | true | 4146 | 4128 | |
| cond_destruct::f | true | 4149 | 4146 | |
| cond_destruct::f | true | 4153 | 4122 | |
| cond_destruct::f | true | 4156 | 4153 | |
| cond_destruct::f | true | 4158 | 4115 | |
| cond_destruct::f | true | 4160 | 4162 | |
| cond_destruct::f | true | 4162 | 4107 | |
| cpp_fun | false | 22196 | 22196 | cpp_fun |
| cpp_fun | false | 22201 | 22201 | declaration |
| cpp_fun | false | 22203 | 22203 | declaration |
Expand Down
20 changes: 20 additions & 0 deletions cpp/ql/test/library-tests/qlcfg/destructors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,23 @@ void destructor_catch() {
HasDtor d2 = { 0 };
}
}

namespace cond_destruct {
struct C {
C();
C(const C&) = delete;
~C();
int getInt() const;
void *data;
};

int f(int x) {
C local;
const C &ref = x ? (const C&)C() : (const C&)local;
return ref.getInt();
// If `x` was true, `ref` refers to a temporary object whose lifetime was
// extended to coincide with `ref`. Before the function returns, it
// should destruct `ref` if and only if the first branch was taken in the
// ?: expression.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, isn't it. @dave-bartolomeo told me yesterday that there were cases with ? : where the compiler would have to create a temporary variable to store which case was taken in order to know whether it should call a destructor. I fiddled around in Compiler Explorer until I got some code with branching around the destructor call. Is that example something like what you had in mind, @dave-bartolomeo?

}
}