Merge pull request #5936 from hvitved/csharp/cfg/perf-tweaks

C#: Various CFG related performance tweaks

Author: hvitved

csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll

@@ -403,7 +403,7 @@ private module JoinBlockPredecessors {
   private import semmle.code.csharp.controlflow.internal.ControlFlowGraphImpl
 
   int getId(JoinBlockPredecessor jbp) {
-    exists(ControlFlowTree::Range t | ControlFlowTree::idOf(t, result) |
+    exists(ControlFlowTree::Range_ t | ControlFlowTree::idOf(t, result) |
       t = jbp.getFirstNode().getElement()
       or
       t = jbp.(EntryBasicBlock).getCallable()

csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll

@@ -2,6 +2,7 @@
 
 import csharp
 private import semmle.code.csharp.ExprOrStmtParent
+private import semmle.code.csharp.commons.Compilation
 private import ControlFlow
 private import ControlFlow::BasicBlocks
 private import SuccessorTypes
@@ -22,7 +23,12 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
   Callable getEnclosingCallable() { none() }
 
   /** Gets the assembly that this element was compiled into. */
-  Assembly getAssembly() { result = this.getEnclosingCallable().getDeclaringType().getALocation() }
+  Assembly getAssembly() {
+    exists(Compilation c |
+      c.getAFileCompiled() = this.getFile() and
+      result = c.getOutputAssembly()
+    )
+  }
 
   /**
    * Gets a control flow node for this element. That is, a node in the

csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll

@@ -1466,8 +1466,10 @@ module Internal {
         PreSsa::Definition def, AssignableRead read
       ) {
         read = def.getAFirstRead() and
-        not exists(AssignableRead other | PreSsa::adjacentReadPairSameVar(other, read) |
-          other != read
+        (
+          not PreSsa::adjacentReadPairSameVar(_, read)
+          or
+          read = unique(AssignableRead read0 | PreSsa::adjacentReadPairSameVar(read0, read))
         )
       }
 
@@ -1651,10 +1653,14 @@ module Internal {
         AssignableRead read1, AssignableRead read2
       ) {
         PreSsa::adjacentReadPairSameVar(read1, read2) and
-        not exists(AssignableRead other |
-          PreSsa::adjacentReadPairSameVar(other, read2) and
-          other != read1 and
-          other != read2
+        (
+          read1 = read2 and
+          read1 = unique(AssignableRead other | PreSsa::adjacentReadPairSameVar(other, read2))
+          or
+          read1 =
+            unique(AssignableRead other |
+              PreSsa::adjacentReadPairSameVar(other, read2) and other != read2
+            )
         )
       }
 
@@ -1887,10 +1893,14 @@ module Internal {
       Ssa::Definition def, ControlFlow::Node cfn1, ControlFlow::Node cfn2
     ) {
       SsaImpl::adjacentReadPairSameVar(def, cfn1, cfn2) and
-      not exists(ControlFlow::Node other |
-        SsaImpl::adjacentReadPairSameVar(def, other, cfn2) and
-        other != cfn1 and
-        other != cfn2
+      (
+        cfn1 = cfn2 and
+        cfn1 = unique(ControlFlow::Node other | SsaImpl::adjacentReadPairSameVar(def, other, cfn2))
+        or
+        cfn1 =
+          unique(ControlFlow::Node other |
+            SsaImpl::adjacentReadPairSameVar(def, other, cfn2) and other != cfn2
+          )
       )
     }
 

csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll

@@ -293,18 +293,6 @@ private class Overflowable extends UnaryOperation {
   }
 }
 
-private class CoreLib extends Assembly {
-  CoreLib() { this = any(SystemExceptionClass c).getALocation() }
-}
-
-/**
- * Holds if assembly `a` was definitely compiled with core library `core`.
- */
-pragma[noinline]
-private predicate assemblyCompiledWithCoreLib(Assembly a, CoreLib core) {
-  a.getAnAttribute().getType().getBaseClass*().(SystemAttributeClass).getALocation() = core
-}
-
 /** A control flow element that is inside a `try` block. */
 private class TriedControlFlowElement extends ControlFlowElement {
   TryStmt try;
@@ -317,7 +305,7 @@ private class TriedControlFlowElement extends ControlFlowElement {
   /**
    * Gets an exception class that is potentially thrown by this element, if any.
    */
-  private Class getAThrownException0() {
+  Class getAThrownException() {
     this instanceof Overflowable and
     result instanceof SystemOverflowExceptionClass
     or
@@ -376,41 +364,6 @@ private class TriedControlFlowElement extends ControlFlowElement {
     this instanceof DynamicExpr and
     result instanceof SystemExceptionClass
   }
-
-  private CoreLib getCoreLibFromACatchClause() {
-    exists(SpecificCatchClause scc | scc = try.getACatchClause() |
-      result = scc.getCaughtExceptionType().getBaseClass*().(SystemExceptionClass).getALocation()
-    )
-  }
-
-  private CoreLib getCoreLib() {
-    result = this.getCoreLibFromACatchClause()
-    or
-    not exists(this.getCoreLibFromACatchClause()) and
-    assemblyCompiledWithCoreLib(this.getAssembly(), result)
-  }
-
-  pragma[noinline]
-  private Class getAThrownExceptionFromPlausibleCoreLib(string name) {
-    result = this.getAThrownException0() and
-    name = result.getQualifiedName() and
-    (
-      not exists(this.getCoreLib())
-      or
-      this.getCoreLib() = result.getALocation()
-    )
-  }
-
-  Class getAThrownException() {
-    exists(string name | result = this.getAThrownExceptionFromPlausibleCoreLib(name) |
-      result =
-        min(Class c |
-          c = this.getAThrownExceptionFromPlausibleCoreLib(name)
-        |
-          c order by c.getLocation().(Assembly).getFullName()
-        )
-    )
-  }
 }
 
 pragma[nomagic]

csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

@@ -77,7 +77,7 @@ class CfgScope extends Element, @top_level_exprorstmt_parent {
 }
 
 module ControlFlowTree {
-  private class Range_ = @callable or @control_flow_element;
+  class Range_ = @callable or @control_flow_element;
 
   class Range extends Element, Range_ {
     Range() { this = getAChild*(any(CfgScope scope)) }
@@ -88,9 +88,9 @@ module ControlFlowTree {
     result = p.(AssignOperation).getExpandedAssignment()
   }
 
-  private predicate id(Range x, Range y) { x = y }
+  private predicate id(Range_ x, Range_ y) { x = y }
 
-  predicate idOf(Range x, int y) = equivalenceRelation(id/2)(x, y)
+  predicate idOf(Range_ x, int y) = equivalenceRelation(id/2)(x, y)
 }
 
 abstract private class ControlFlowTree extends ControlFlowTree::Range {

csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll

@@ -39,8 +39,10 @@ private class ThrowingCall extends NonReturningCall {
       or
       this.(FailingAssertion).getAssertionFailure().isException(c.getExceptionClass())
       or
-      exists(CIL::Method m, CIL::Type ex |
-        this.getTarget().matchesHandle(m) and
+      exists(Callable target, CIL::Method m, CIL::Type ex |
+        target = this.getTarget() and
+        not target.hasBody() and
+        target.matchesHandle(m) and
         alwaysThrowsException(m, ex) and
         c.getExceptionClass().matchesHandle(ex) and
         not m.isVirtual()

csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll

@@ -209,6 +209,13 @@ abstract class SplitImpl extends Split {
     or
     exists(ControlFlowElement pred | this.appliesTo(pred) | this.hasSuccessor(pred, cfe, _))
   }
+
+  /** The `succ` relation restricted to predecessors `pred` that this split applies to. */
+  pragma[noinline]
+  final predicate appliesSucc(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
+    this.appliesTo(pred) and
+    succ(pred, succ, c)
+  }
 }
 
 module InitializerSplitting {
@@ -382,8 +389,7 @@ module InitializerSplitting {
     }
 
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-      this.appliesTo(pred) and
-      succ(pred, succ, c) and
+      this.appliesSucc(pred, succ, c) and
       succ =
         any(InitializedInstanceMember m |
           constructorInitializes(this.getConstructor(), m.getInitializer())
@@ -620,8 +626,7 @@ module AssertionSplitting {
     }
 
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-      this.appliesTo(pred) and
-      succ(pred, succ, c) and
+      this.appliesSucc(pred, succ, c) and
       succ = getAnAssertionDescendant(a)
     }
   }
@@ -858,8 +863,7 @@ module FinallySplitting {
     }
 
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
-      this.appliesToPredecessor(pred) and
-      succ(pred, succ, c) and
+      this.appliesSucc(pred, succ, c) and
       succ =
         any(FinallyControlFlowElement fcfe |
           if fcfe.isEntryNode()
@@ -1037,7 +1041,7 @@ module ExceptionHandlerSplitting {
 
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
       this.appliesToPredecessor(pred, c) and
-      succ(pred, succ, c) and
+      this.appliesSucc(pred, succ, c) and
       not first(any(SpecificCatchClause scc).getBlock(), succ) and
       not succ instanceof GeneralCatchClause and
       not exists(TryStmt ts, SpecificCatchClause scc, int last |
@@ -1298,7 +1302,7 @@ module BooleanSplitting {
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
       exists(PreBasicBlock bb, Completion c0 | this.appliesToBlock(bb, c0) |
         pred = bb.getAnElement() and
-        succ(pred, succ, c) and
+        this.appliesSucc(pred, succ, c) and
         (
           pred = bb.getLastElement()
           implies
@@ -1489,7 +1493,7 @@ module LoopSplitting {
 
     override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
       this.appliesToPredecessor(pred, c) and
-      succ(pred, succ, c) and
+      this.appliesSucc(pred, succ, c) and
       not loop.stop(pred, succ, c)
     }
   }

csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplSpecific.qll

@@ -15,13 +15,37 @@ class ExitBasicBlock extends BasicBlock {
   ExitBasicBlock() { scopeLast(_, this.getLastElement(), _) }
 }
 
-pragma[noinline]
+/** Holds if `a` is assigned in non-constructor callable `c`. */
+pragma[nomagic]
+private predicate assignableDefinition(Assignable a, Callable c) {
+  exists(AssignableDefinition def | def.getTarget() = a |
+    c = def.getEnclosingCallable() and
+    not c instanceof Constructor
+  )
+}
+
+/** Holds if `a` is accessed in callable `c`. */
+pragma[nomagic]
+private predicate assignableAccess(Assignable a, Callable c) {
+  exists(AssignableAccess aa | aa.getTarget() = a | c = aa.getEnclosingCallable())
+}
+
+pragma[nomagic]
 private predicate assignableNoCapturing(Assignable a, Callable c) {
-  exists(AssignableAccess aa | aa.getTarget() = a | c = aa.getEnclosingCallable()) and
-  forall(AssignableDefinition def | def.getTarget() = a |
-    c = def.getEnclosingCallable()
+  assignableAccess(a, c) and
+  /*
+   * The code below is equivalent to
+   * ```ql
+   * not exists(Callable other | assignableDefinition(a, other) | other != c)
+   * ```
+   * but it avoids a Cartesian product in the compiler generated antijoin
+   * predicate.
+   */
+
+  (
+    not assignableDefinition(a, _)
     or
-    def.getEnclosingCallable() instanceof Constructor
+    c = unique(Callable c0 | assignableDefinition(a, c0) | c0)
   )
 }
 
@@ -41,7 +65,7 @@ class SourceVariable extends Assignable {
     (
       this instanceof LocalScopeVariable
       or
-      this instanceof Field
+      this = any(Field f | not f.isVolatile())
       or
       this = any(TrivialProperty tp | not tp.isOverridableOrImplementable())
     ) and

csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll

@@ -49,7 +49,7 @@ module Private {
   }
 
   int getId(PhiInputEdgeBlock bb) {
-    exists(CfgImpl::ControlFlowTree::Range t | CfgImpl::ControlFlowTree::idOf(t, result) |
+    exists(CfgImpl::ControlFlowTree::Range_ t | CfgImpl::ControlFlowTree::idOf(t, result) |
       t = bb.getFirstNode().getElement()
       or
       t = bb.(CS::ControlFlow::BasicBlocks::EntryBlock).getCallable()