Merge pull request #995 from hvitved/csharp/split-guards-performance

C#: Speedup guards predicates
2026-05-04 21:25:44 +02:00 · 2019-03-05 16:42:39 +00:00
parent b3d935063f 4cbbe37b1e
commit b17de116f8
6 changed files with 153 additions and 51 deletions
--- a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll
+++ b/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll
@@ -42,41 +42,81 @@ class Assertion extends MethodCall {
  /** Gets the expression that this assertion pertains to. */
  Expr getExpr() { result = this.getArgumentForParameter(target.getAssertedParameter()) }

+  /**
+   * Holds if basic block `succ` is immediately dominated by this assertion.
+   * That is, `succ` can only be reached from the callable entry point by
+   * going via *some* basic block `pred` containing this assertion, and `pred`
+   * is an immediate predecessor of `succ`.
+   *
+   * Moreover, this assertion corresponds to multiple control flow nodes,
+   * which is why
+   *
+   * ```
+   * exists(BasicBlock bb |
+   *   bb.getANode() = this.getAControlFlowNode() |
+   *   bb.immediatelyDominates(succ)
+   * )
+   * ```
+   *
+   * does not work.
+   */
  pragma[nomagic]
-  private JoinBlockPredecessor getAPossiblyDominatedPredecessor(JoinBlock jb) {
+  private predicate immediatelyDominatesBlockSplit(BasicBlock succ) {
    // Only calculate dominance by explicit recursion for split nodes;
    // all other nodes can use regular CFG dominance
    this instanceof ControlFlow::Internal::SplitControlFlowElement and
-    exists(BasicBlock bb | bb = this.getAControlFlowNode().getBasicBlock() |
-      result = bb.getASuccessor*()
-    ) and
-    result.getASuccessor() = jb and
-    not jb.dominates(result)
+    exists(BasicBlock bb | bb.getANode() = this.getAControlFlowNode() |
+      succ = bb.getASuccessor() and
+      forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != bb |
+        succ.dominates(pred)
+        or
+        // `pred` might be another split of this element
+        pred.getANode().getElement() = this
+      )
+    )
  }

-  pragma[nomagic]
-  private predicate isPossiblyDominatedJoinBlock(JoinBlock jb) {
-    exists(this.getAPossiblyDominatedPredecessor(jb)) and
-    forall(BasicBlock pred | pred = jb.getAPredecessor() |
-      pred = this.getAPossiblyDominatedPredecessor(jb)
+  pragma[noinline]
+  private predicate strictlyDominatesJoinBlockPredecessor(JoinBlock jb, int i) {
+    this.strictlyDominatesSplit(jb.getJoinBlockPredecessor(i))
+  }
+
+  private predicate strictlyDominatesJoinBlockSplit(JoinBlock jb, int i) {
+    i = -1 and
+    this.strictlyDominatesJoinBlockPredecessor(jb, _)
+    or
+    this.strictlyDominatesJoinBlockSplit(jb, i - 1) and
+    (
+      this.strictlyDominatesJoinBlockPredecessor(jb, i)
      or
-      jb.dominates(pred)
+      this.getAControlFlowNode().getBasicBlock() = jb.getJoinBlockPredecessor(i)
    )
  }

  pragma[nomagic]
  private predicate strictlyDominatesSplit(BasicBlock bb) {
-    this.getAControlFlowNode().getBasicBlock().immediatelyDominates(bb)
+    this.immediatelyDominatesBlockSplit(bb)
    or
-    if bb instanceof JoinBlock
-    then
-      this.isPossiblyDominatedJoinBlock(bb) and
-      forall(BasicBlock pred | pred = this.getAPossiblyDominatedPredecessor(bb) |
-        this.strictlyDominatesSplit(pred)
-        or
-        this.getAControlFlowNode().getBasicBlock() = pred
-      )
-    else this.strictlyDominatesSplit(bb.getAPredecessor())
+    // Equivalent with
+    //
+    // ```
+    // exists(JoinBlockPredecessor pred | pred = bb.getAPredecessor() |
+    //   this.strictlyDominatesSplit(pred)
+    // ) and
+    // forall(JoinBlockPredecessor pred | pred = bb.getAPredecessor() |
+    //   this.strictlyDominatesSplit(pred)
+    //   or
+    //   this.getAControlFlowNode().getBasicBlock() = pred
+    // )
+    // ```
+    //
+    // but uses no universal recursion for better performance.
+    exists(int last | last = max(int i | exists(bb.(JoinBlock).getJoinBlockPredecessor(i))) |
+      this.strictlyDominatesJoinBlockSplit(bb, last)
+    )
+    or
+    not bb instanceof JoinBlock and
+    this.strictlyDominatesSplit(bb.getAPredecessor())
  }

  /**
--- a/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll
+++ b/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll
@@ -407,9 +407,47 @@ class ExitBasicBlock extends BasicBlock {
 /** Holds if `bb` is an exit basic block. */
 private predicate exitBB(BasicBlock bb) { bb.getLastNode() instanceof ControlFlow::Nodes::ExitNode }

+private module JoinBlockPredecessors {
+  private import ControlFlow::Nodes
+
+  private class CallableOrCFE extends Element {
+    CallableOrCFE() { this instanceof Callable or this instanceof ControlFlowElement }
+  }
+
+  private predicate id(CallableOrCFE x, CallableOrCFE y) { x = y }
+
+  private predicate idOf(CallableOrCFE x, int y) = equivalenceRelation(id/2)(x, y)
+
+  int getId(JoinBlockPredecessor jbp) {
+    idOf(jbp.getFirstNode().(ElementNode).getElement(), result)
+    or
+    idOf(jbp.(EntryBasicBlock).getCallable(), result)
+  }
+
+  string getSplitString(JoinBlockPredecessor jbp) {
+    result = jbp.getFirstNode().(ElementNode).getSplitsString()
+    or
+    not exists(jbp.getFirstNode().(ElementNode).getSplitsString()) and
+    result = ""
+  }
+}
+
 /** A basic block with more than one predecessor. */
 class JoinBlock extends BasicBlock {
  JoinBlock() { getFirstNode().isJoin() }
+
+  /**
+   * Gets the `i`th predecessor of this join block, with respect to some
+   * arbitrary order.
+   */
+  cached
+  JoinBlockPredecessor getJoinBlockPredecessor(int i) {
+    result = rank[i + 1](JoinBlockPredecessor jbp |
+        jbp = this.getAPredecessor()
+      |
+        jbp order by JoinBlockPredecessors::getId(jbp), JoinBlockPredecessors::getSplitString(jbp)
+      )
+  }
 }

 /** A basic block that is an immediate predecessor of a join block. */
--- a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll
+++ b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll
@@ -101,32 +101,27 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
      |
        succ.dominates(pred)
        or
-        // `pred` might be another split of `cfe`
+        // `pred` might be another split of this element
        pred.getLastNode().getElement() = this and
-        pred.getASuccessorByType(t) = succ and
        t = s
      )
    )
  }

-  pragma[nomagic]
-  private JoinBlockPredecessor getAPossiblyControlledPredecessor(
-    JoinBlock controlled, ConditionalSuccessor s
-  ) {
-    exists(BasicBlock mid | this.immediatelyControlsBlockSplit(mid, s) |
-      result = mid.getASuccessor*()
-    ) and
-    result.getASuccessor() = controlled and
-    not controlled.dominates(result)
+  pragma[noinline]
+  private predicate controlsJoinBlockPredecessor(JoinBlock controlled, ConditionalSuccessor s, int i) {
+    this.controlsBlockSplit(controlled.getJoinBlockPredecessor(i), s)
  }

-  pragma[nomagic]
-  private predicate isPossiblyControlledJoinBlock(JoinBlock controlled, ConditionalSuccessor s) {
-    exists(this.getAPossiblyControlledPredecessor(controlled, s)) and
-    forall(BasicBlock pred | pred = controlled.getAPredecessor() |
-      pred = this.getAPossiblyControlledPredecessor(controlled, s)
+  private predicate controlsJoinBlockSplit(JoinBlock controlled, ConditionalSuccessor s, int i) {
+    i = -1 and
+    this.controlsJoinBlockPredecessor(controlled, s, _)
+    or
+    this.controlsJoinBlockSplit(controlled, s, i - 1) and
+    (
+      this.controlsJoinBlockPredecessor(controlled, s, i)
      or
-      controlled.dominates(pred)
+      controlled.dominates(controlled.getJoinBlockPredecessor(i))
    )
  }

@@ -134,13 +129,28 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
  private predicate controlsBlockSplit(BasicBlock controlled, ConditionalSuccessor s) {
    this.immediatelyControlsBlockSplit(controlled, s)
    or
-    if controlled instanceof JoinBlock
-    then
-      this.isPossiblyControlledJoinBlock(controlled, s) and
-      forall(BasicBlock pred | pred = this.getAPossiblyControlledPredecessor(controlled, s) |
-        this.controlsBlock(pred, s)
-      )
-    else this.controlsBlockSplit(controlled.getAPredecessor(), s)
+    // Equivalent with
+    //
+    // ```
+    // exists(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() |
+    //   this.controlsBlockSplit(pred, s)
+    // ) and
+    // forall(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() |
+    //   this.controlsBlockSplit(pred, s)
+    //   or
+    //   controlled.dominates(pred)
+    // )
+    // ```
+    //
+    // but uses no universal recursion for better performance.
+    exists(int last |
+      last = max(int i | exists(controlled.(JoinBlock).getJoinBlockPredecessor(i)))
+    |
+      this.controlsJoinBlockSplit(controlled, s, last)
+    )
+    or
+    not controlled instanceof JoinBlock and
+    this.controlsBlockSplit(controlled.getAPredecessor(), s)
  }

  /**
--- a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
+++ b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
@@ -1271,6 +1271,14 @@ module Internal {
      exists(ConditionOnExprComparisonConfig c | c.same(sub, guarded.getElement()))
    }

+    pragma[noinline]
+    private predicate isGuardedByNode2(ControlFlow::Nodes::ElementNode guarded, Ssa::Definition def) {
+      isGuardedByNode1(guarded, _, _, _) and
+      exists(BasicBlock bb | bb = guarded.getBasicBlock() |
+        def = guarded.getElement().(AccessOrCallExpr).getAnSsaQualifier(bb.getANode())
+      )
+    }
+
    cached
    predicate isGuardedByNode(
      ControlFlow::Nodes::ElementNode guarded, Guard g, AccessOrCallExpr sub, AbstractValue v
@@ -1278,11 +1286,7 @@ module Internal {
      isGuardedByNode1(guarded, g, sub, v) and
      sub = g.getAChildExpr*() and
      forall(Ssa::Definition def | def = sub.getAnSsaQualifier(_) |
-        exists(ControlFlow::Node cfn |
-          def = guarded.getElement().(AccessOrCallExpr).getAnSsaQualifier(cfn)
-        |
-          cfn.getBasicBlock() = guarded.getBasicBlock()
-        )
+        isGuardedByNode2(guarded, def)
      )
    }

--- a/csharp/ql/test/library-tests/comments/Generics.expected
+++ b/csharp/ql/test/library-tests/comments/Generics.expected
@@ -1,4 +1,5 @@
 | comments2.cs:118:5:118:21 | // ... | comments2.cs:119:11:119:25 | GenericClass<> | NestedType |
 | comments2.cs:118:5:118:21 | // ... | comments2.cs:119:11:119:25 | GenericClass<> | UnboundGenericClass |
+| comments2.cs:124:5:124:16 | // ... | comments2.cs:125:9:125:20 | GenericFn | CallableOrCFE |
 | comments2.cs:124:5:124:16 | // ... | comments2.cs:125:9:125:20 | GenericFn | InstanceCallable |
 | comments2.cs:124:5:124:16 | // ... | comments2.cs:125:9:125:20 | GenericFn | UnboundGenericMethod |
--- a/csharp/ql/test/library-tests/frameworks/test/Test.expected
+++ b/csharp/ql/test/library-tests/frameworks/test/Test.expected
@@ -1,24 +1,33 @@
 | VisualStudio.cs:9:11:9:21 | MyTestSuite | TestClass | LeafType |
 | VisualStudio.cs:9:11:9:21 | MyTestSuite | TestClass | VSTestClass |
+| VisualStudio.cs:12:21:12:25 | Test1 | TestMethod | CallableOrCFE |
 | VisualStudio.cs:12:21:12:25 | Test1 | TestMethod | InstanceCallable |
 | VisualStudio.cs:12:21:12:25 | Test1 | TestMethod | VSTestMethod |
+| VisualStudio.cs:17:21:17:25 | Test2 | TestMethod | CallableOrCFE |
 | VisualStudio.cs:17:21:17:25 | Test2 | TestMethod | InstanceCallable |
 | VisualStudio.cs:17:21:17:25 | Test2 | TestMethod | VSTestMethod |
 | XUnit.cs:22:11:22:21 | MyTestSuite | TestClass | LeafType |
 | XUnit.cs:22:11:22:21 | MyTestSuite | TestClass | XUnitTestClass |
+| XUnit.cs:25:21:25:25 | Test1 | TestMethod | CallableOrCFE |
 | XUnit.cs:25:21:25:25 | Test1 | TestMethod | InstanceCallable |
 | XUnit.cs:25:21:25:25 | Test1 | TestMethod | XUnitTestMethod |
+| XUnit.cs:30:21:30:25 | Test2 | TestMethod | CallableOrCFE |
 | XUnit.cs:30:21:30:25 | Test2 | TestMethod | InstanceCallable |
 | XUnit.cs:30:21:30:25 | Test2 | TestMethod | XUnitTestMethod |
 | nunit.cs:42:11:42:21 | MyTestSuite | TestClass | LeafType |
 | nunit.cs:42:11:42:21 | MyTestSuite | TestClass | NUnitFixture |
+| nunit.cs:52:21:52:25 | Test1 | TestMethod | CallableOrCFE |
 | nunit.cs:52:21:52:25 | Test1 | TestMethod | InstanceCallable |
 | nunit.cs:52:21:52:25 | Test1 | TestMethod | NUnitTestMethod |
+| nunit.cs:57:21:57:25 | Test2 | TestMethod | CallableOrCFE |
 | nunit.cs:57:21:57:25 | Test2 | TestMethod | InstanceCallable |
 | nunit.cs:57:21:57:25 | Test2 | TestMethod | NUnitTestMethod |
+| nunit.cs:62:21:62:25 | Test3 | TestMethod | CallableOrCFE |
 | nunit.cs:62:21:62:25 | Test3 | TestMethod | InstanceCallable |
 | nunit.cs:62:21:62:25 | Test3 | TestMethod | NUnitTestMethod |
+| nunit.cs:67:21:67:25 | Test4 | TestMethod | CallableOrCFE |
 | nunit.cs:67:21:67:25 | Test4 | TestMethod | InstanceCallable |
 | nunit.cs:67:21:67:25 | Test4 | TestMethod | NUnitTestMethod |
+| nunit.cs:72:21:72:25 | Test5 | TestMethod | CallableOrCFE |
 | nunit.cs:72:21:72:25 | Test5 | TestMethod | InstanceCallable |
 | nunit.cs:72:21:72:25 | Test5 | TestMethod | NUnitTestMethod |