Skip to content

Commit b4385c6

Browse files
committed
C++: Don't use GVN in AST DataFlow BarrierNode
It turns out that the evaluator will evaluate the GVN stage even when no predicate from it is needed after optimization of the subsequent stages. The GVN library is expensive to evaluate, and it'll become even more expensive when we switch its implementation to IR. This PR disables the use of GVN in `DataFlow::BarrierNode` for the AST data-flow library, which should improve performance when evaluating a single data-flow query on a snapshot with no cache. Precision decreases slightly, leading to a new FP in the qltests. There is no corresponding change for the IR data-flow library since IR GVN is not very expensive.
1 parent 3a7845e commit b4385c6

File tree

4 files changed

+6
-5
lines changed

4 files changed

+6
-5
lines changed

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ private import cpp
66
private import semmle.code.cpp.dataflow.internal.FlowVar
77
private import semmle.code.cpp.models.interfaces.DataFlow
88
private import semmle.code.cpp.controlflow.Guards
9-
private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
109

1110
cached
1211
private newtype TNode =
@@ -689,9 +688,9 @@ class BarrierGuard extends GuardCondition {
689688

690689
/** Gets a node guarded by this guard. */
691690
final ExprNode getAGuardedNode() {
692-
exists(GVN value, boolean branch |
693-
result.getExpr() = value.getAnExpr() and
694-
this.checks(value.getAnExpr(), branch) and
691+
exists(SsaDefinition def, Variable v, boolean branch |
692+
result.getExpr() = def.getAUse(v) and
693+
this.checks(def.getAUse(v), branch) and
695694
this.controls(result.getExpr().getBasicBlock(), branch)
696695
)
697696
}

cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ struct XY {
4848
void bg_stackstruct(XY s1, XY s2) {
4949
s1.x = source();
5050
if (guarded(s1.x)) {
51-
sink(s1.x); // no flow
51+
sink(s1.x); // no flow [FALSE POSITIVE in AST]
5252
} else if (guarded(s1.y)) {
5353
sink(s1.x); // flow
5454
} else if (guarded(s2.y)) {

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
| BarrierGuard.cpp:25:10:25:15 | source | BarrierGuard.cpp:21:17:21:22 | source |
44
| BarrierGuard.cpp:31:10:31:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
55
| BarrierGuard.cpp:33:10:33:15 | source | BarrierGuard.cpp:29:16:29:21 | source |
6+
| BarrierGuard.cpp:51:13:51:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
67
| BarrierGuard.cpp:53:13:53:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
78
| BarrierGuard.cpp:55:13:55:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source |
89
| BarrierGuard.cpp:62:14:62:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
| BarrierGuard.cpp:49:10:49:15 | BarrierGuard.cpp:51:13:51:13 | AST only |
12
| BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only |
23
| clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only |
34
| clang.cpp:28:27:28:32 | clang.cpp:30:27:30:34 | AST only |

0 commit comments

Comments
 (0)