From b4385c6e605031c5ee615cbad24413fa9abdc813 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 4 Feb 2020 08:40:36 +0100 Subject: [PATCH] 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. --- .../src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll | 7 +++---- .../library-tests/dataflow/dataflow-tests/BarrierGuard.cpp | 2 +- .../library-tests/dataflow/dataflow-tests/test.expected | 1 + .../dataflow/dataflow-tests/test_diff.expected | 1 + 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index 2c3438a1e46a..700087871ccc 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -6,7 +6,6 @@ private import cpp private import semmle.code.cpp.dataflow.internal.FlowVar private import semmle.code.cpp.models.interfaces.DataFlow private import semmle.code.cpp.controlflow.Guards -private import semmle.code.cpp.valuenumbering.GlobalValueNumbering cached private newtype TNode = @@ -689,9 +688,9 @@ class BarrierGuard extends GuardCondition { /** Gets a node guarded by this guard. */ final ExprNode getAGuardedNode() { - exists(GVN value, boolean branch | - result.getExpr() = value.getAnExpr() and - this.checks(value.getAnExpr(), branch) and + exists(SsaDefinition def, Variable v, boolean branch | + result.getExpr() = def.getAUse(v) and + this.checks(def.getAUse(v), branch) and this.controls(result.getExpr().getBasicBlock(), branch) ) } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp index f2b91c00ed04..1896a066f682 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp @@ -48,7 +48,7 @@ struct XY { void bg_stackstruct(XY s1, XY s2) { s1.x = source(); if (guarded(s1.x)) { - sink(s1.x); // no flow + sink(s1.x); // no flow [FALSE POSITIVE in AST] } else if (guarded(s1.y)) { sink(s1.x); // flow } else if (guarded(s2.y)) { diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected index 24fa6fdb5bd9..04ad48cd4d67 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected @@ -3,6 +3,7 @@ | BarrierGuard.cpp:25:10:25:15 | source | BarrierGuard.cpp:21:17:21:22 | source | | BarrierGuard.cpp:31:10:31:15 | source | BarrierGuard.cpp:29:16:29:21 | source | | BarrierGuard.cpp:33:10:33:15 | source | BarrierGuard.cpp:29:16:29:21 | source | +| BarrierGuard.cpp:51:13:51:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source | | BarrierGuard.cpp:53:13:53:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source | | BarrierGuard.cpp:55:13:55:13 | x | BarrierGuard.cpp:49:10:49:15 | call to source | | BarrierGuard.cpp:62:14:62:14 | x | BarrierGuard.cpp:60:11:60:16 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected index 31639764ad67..8daa9b4b39bc 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected @@ -1,3 +1,4 @@ +| BarrierGuard.cpp:49:10:49:15 | BarrierGuard.cpp:51:13:51:13 | AST only | | BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only | | clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only | | clang.cpp:28:27:28:32 | clang.cpp:30:27:30:34 | AST only |