C++: reachableRecursive refactor for performance

The `reachable` predicate is large and slow to compute. It's part of a
mutual recursion that's non-linear, meaning it has a recursive call on
both sides of an `and`.

This change removes a part of the base case that has no effect on
recursive cases. The removed part is added back after the recursion has
finished.

Before, on Wireshark:

    ControlFlowGraph::Cached::reachable#f .......... 20.8s (executed 9800 times)
    ConstantExprs::successors_adapted#ff ........... 4.2s (executed 615 times)
    ConstantExprs::potentiallyReturningFunction#f .. 3.9s (executed 9799 times)
    ConstantExprs::possiblePredecessor#f ........... 2.9s (executed 788 times)

After, on Wireshark:

    ConstantExprs::reachableRecursive#f ............ 13.2s (executed 9800 times)
    ConstantExprs::successors_adapted#ff ........... 4.2s (executed 615 times)
    ConstantExprs::potentiallyReturningFunction#f .. 4.3s (executed 9799 times)
    ConstantExprs::possiblePredecessor#f ........... 2.6s (executed 788 times)

I've verified that this change doesn't change what's computed by
checking that the output of the following query is unchanged:

    import cpp
    import semmle.code.cpp.controlflow.internal.ConstantExprs

    select
      strictcount(ControlFlowNode n | reachable(n)) as reachable,
      strictcount(ControlFlowNode n1, ControlFlowNode n2 | n2 = n1.getASuccessor()) as edges,
      strictcount(FunctionCall c | aborting(c)) as abortingCall,
      strictcount(Function f | abortingFunction(f)) as abortingFunction

Author: jbj

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

@@ -44,23 +44,6 @@ private module Cached {
    */
   cached
   module ControlFlowGraphPublic {
-    // The base case of `reachable` is factored out for performance. If it's
-    // written in-line in `reachable`, the compiler inserts a `n instanceof
-    // ControlFlowNode` check because the `not ... and not ...` case doesn't
-    // otherwise bind `n`. The problem is that this check is inserted at the
-    // outermost level of this predicate, so it covers all cases including the
-    // recursive case. The optimizer doesn't eliminate the check even though it's
-    // redundant, and having the check leads to needless extra computation and a
-    // risk of bad join orders.
-    private predicate reachableBaseCase(ControlFlowNode n) {
-      exists(Function f | f.getEntryPoint() = n)
-      or
-      // Okay to use successors_extended directly here
-      (not successors_extended(_,n) and not successors_extended(n,_))
-      or
-      n instanceof Handler
-    }
-
     /**
      * Holds if the control-flow node `n` is reachable, meaning that either
      * it is an entry point, or there exists a path in the control-flow
@@ -72,9 +55,10 @@ private module Cached {
     cached
     predicate reachable(ControlFlowNode n)
     {
-      reachableBaseCase(n)
+      // Okay to use successors_extended directly here
+      reachableRecursive(n)
       or
-      reachable(n.getAPredecessor())
+      (not successors_extended(_,n) and not successors_extended(n,_))
     }
 
     /** Holds if `condition` always evaluates to a nonzero value. */
@@ -169,7 +153,7 @@ private predicate potentiallyReturningFunction(Function f) {
     nonAnalyzableFunction(f)
     or
     // otherwise the exit-point of `f` must be reachable
-    reachable(f)
+    reachableRecursive(f)
   )
 }
 
@@ -309,6 +293,23 @@ private predicate possiblePredecessor(Node pred) {
   potentiallyReturningFunctionCall(pred)
 }
 
+/**
+ * Holds if the control-flow node `n` is reachable, meaning that either
+ * it is an entry point, or there exists a path in the control-flow
+ * graph of its function that connects an entry point to it.
+ * Compile-time constant conditions are taken into account, so that
+ * the call to `f` is not reachable in `if (0) f();` even if the
+ * `if` statement as a whole is reachable.
+ */
+private predicate reachableRecursive(ControlFlowNode n)
+{
+  exists(Function f | f.getEntryPoint() = n)
+  or
+  n instanceof Handler
+  or
+  reachableRecursive(n.getAPredecessor())
+}
+
 private predicate compileTimeConstantInt(Expr e, int val) {
   val = e.getFullyConverted().getValue().toInt() and
   not e instanceof StringLiteral and