C++: Don't use dbtypes in ControlFlowNode etc.

Many classes have been declared with `extends @cfgnode` because they
should be implemented internally as a control-flow node but should not
expose the member predicates of `ControlFlowNode` to their users. After
the transition in a1e4404 it became mandatory to convert explicitly
between the `Element`-derived `ControlFlowNode` and the raw dbtype
`@cfgnode`, and that commit inserted numerous such conversions as a
result of having all those classes that did not derive from `Element` in
the standard library.

It was also confusing and error-prone that the libraries implementing
`ControlFlowNode` referred to `ControlFlowNode`. This seemingly cyclic
reference worked out because the libraries did not call the predicates
on `ControlFlowNode` whose implementation they were part of.

Both these problems are now solved by adding a new class
`ControlFlowNodeBase extends Element` that should be used in preference
to `@cfgnode` everywhere. This class is for exactly those use cases
where `@cfgnode` should be seen as an `Element` without having too many
member predicates on it.

The classes that move from extending `@cfgnode` to extending
`ControlFlowNodeBase` are: `BasicBlock`, `AdditionalControlFlowEdge`,
`DefOrUse`, `SsaDefinition`, `SubBasicBlock` and `RangeSsaDefinition`.
These previously had to define their own `toString` rootdef, which
typically had some dummy string as result (like `"BasicBlock"`), but now
their `toString` is part of the `Element` rootdef and should not be
overridden otherwise `Element.toString` will sometimes have multiple
results. Removing these dummy `toString` predicates had some effects on
the tests that are included in this commit.

The `getLocation` family of predicates is affected like `toString`, but
the situation is slightly different. Some of these classes had genuinely
useful alternative definitions of locations. Fortunately, they all used
`hasLocationInfo`, which is preferred over `getLocation` by the QL
engine. Because `Element` does not define `getLocationInfo`, each class
can create its own rootdef of this predicate like before.

Author: jbj

cpp/ql/src/Likely Bugs/Format/SnprintfOverflow.ql

@@ -98,7 +98,7 @@ predicate flowsToDefImpl(
   or
   // `x++`
   exists (CrementOperation crem
-  | mkElement(def) = crem and
+  | def = crem and
     crem.getOperand() = v.getAnAccess() and
     flowsToExpr(source, crem.getOperand(), pathMightOverflow))
   or

cpp/ql/src/Security/CWE/CWE-764/LockFlow.qll

@@ -27,7 +27,7 @@ predicate tryLockCondition(VariableAccess access,
     (cond = call.getParent*() and
      cond.isCondition() and
      failNode = cond.getASuccessor() and
-     unresolveElement(failNode) instanceof BasicBlockWithReturn))
+     failNode instanceof BasicBlockWithReturn))
 }
 
 /**

cpp/ql/src/Security/CWE/CWE-764/UnreleasedLock.ql

@@ -29,7 +29,7 @@ predicate failedLock(MutexType t, BasicBlock lockblock, BasicBlock failblock) {
   exists (ControlFlowNode lock |
     lock = lockblock.getEnd() and
     lock = t.getLockAccess() and
-    lock.getAFalseSuccessor() = mkElement(failblock)
+    lock.getAFalseSuccessor() = failblock
   )
 }
 

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

@@ -60,7 +60,7 @@ private cached module Cached {
   private predicate non_primitive_basic_block_entry_node(ControlFlowNode node) {
     not primitive_basic_block_entry_node(node) and
     not exists(node.getAPredecessor()) and
-    successors_extended(unresolveElement(node), _)
+    successors_extended(node, _)
   }
 
   /**
@@ -80,7 +80,7 @@ private cached module Cached {
    * reuse predicates already computed for `PrimitiveBasicBlocks`.
    */
   private predicate equalsPrimitiveBasicBlock(BasicBlock bb) {
-    primitive_basic_block_entry_node(mkElement(bb))
+    primitive_basic_block_entry_node(bb)
     and
     not exists(int i |
       i > 0 and
@@ -96,11 +96,11 @@ private cached module Cached {
   }
 
   private predicate non_primitive_basic_block_member(ControlFlowNode node, BasicBlock bb, int pos) {
-    (not equalsPrimitiveBasicBlock(bb) and node = mkElement(bb) and pos = 0)
+    (not equalsPrimitiveBasicBlock(bb) and node = bb and pos = 0)
     or
-    (not (unresolveElement(node) instanceof BasicBlock) and
+    (not (node instanceof BasicBlock) and
      exists (ControlFlowNode pred
-     | successors_extended(unresolveElement(pred),unresolveElement(node))
+     | successors_extended(pred, node)
      | non_primitive_basic_block_member(pred, bb, pos - 1)))
   }
 
@@ -117,7 +117,7 @@ private cached module Cached {
   predicate bb_successor_cached(BasicBlock pred, BasicBlock succ) {
     exists(ControlFlowNode last |
       basic_block_member(last, pred, bb_length(pred)-1) and
-      last.getASuccessor() = mkElement(succ)
+      last.getASuccessor() = succ
     )
   }
 }
@@ -143,15 +143,10 @@ predicate bb_successor = bb_successor_cached/2;
  *    A - B < C - D  AB is a basic block and CD is a basic block (B has two outgoing edges)
  * ```
  */
-class BasicBlock extends @cfgnode {
+class BasicBlock extends ControlFlowNodeBase {
 
   BasicBlock() {
-    basic_block_entry_node(mkElement(this))
-  }
-
-  /** Gets a textual representation of this element. */
-  string toString() {
-    result = "BasicBlock"
+    basic_block_entry_node(this)
   }
 
   predicate contains(ControlFlowNode node) {
@@ -187,7 +182,7 @@ class BasicBlock extends @cfgnode {
   }
 
   ControlFlowNode getStart() {
-    result = mkElement(this)
+    result = this
   }
 
   /** Gets the number of `ControlFlowNode`s in this basic block. */
@@ -248,9 +243,9 @@ class BasicBlock extends @cfgnode {
    * point or a `catch` clause of a reachable `try` statement.
    */
   predicate isReachable() {
-    exists(Function f | f.getBlock() = mkElement(this))
+    exists(Function f | f.getBlock() = this)
     or
-    exists(TryStmt t, BasicBlock tryblock | mkElement(this) = t.getACatchClause() and tryblock.isReachable() and tryblock.contains(t))
+    exists(TryStmt t, BasicBlock tryblock | this = t.getACatchClause() and tryblock.isReachable() and tryblock.contains(t))
     or
     exists(BasicBlock pred | pred.getASuccessor() = this and pred.isReachable())
   }
@@ -272,7 +267,7 @@ predicate unreachable(ControlFlowNode n) {
  */
 class EntryBasicBlock extends BasicBlock {
   EntryBasicBlock() {
-    exists (Function f | mkElement(this) = f.getEntryPoint())
+    exists (Function f | this = f.getEntryPoint())
   }
 }
 

cpp/ql/src/semmle/code/cpp/controlflow/ControlFlowGraph.qll

@@ -27,9 +27,8 @@ private import semmle.code.cpp.controlflow.internal.ConstantExprs
  * function, or to the exit point of the function if there is no such
  * `Handler`. There are no edges from function calls to `Handler`s.
  */
-class ControlFlowNode extends Locatable, @cfgnode {
-
-  ControlFlowNode getASuccessor() { successors_adapted(underlyingElement(this),unresolveElement(result)) }
+class ControlFlowNode extends Locatable, ControlFlowNodeBase {
+  ControlFlowNode getASuccessor() { successors_adapted(this, result) }
 
   ControlFlowNode getAPredecessor() { this = result.getASuccessor() }
 
@@ -58,15 +57,17 @@ class ControlFlowNode extends Locatable, @cfgnode {
    * taken when this expression is true.
    */
   ControlFlowNode getATrueSuccessor() {
-    truecond(underlyingElement(this),unresolveElement(result)) and result = getASuccessor()
+    truecond_base(this, result) and
+    result = getASuccessor()
   }
 
   /**
    * Gets a node such that the control-flow edge `(this, result)` may be
    * taken when this expression is false.
    */
   ControlFlowNode getAFalseSuccessor() {
-    falsecond(underlyingElement(this),unresolveElement(result)) and result = getASuccessor()
+    falsecond_base(this,result) and
+    result = getASuccessor()
   }
 
   BasicBlock getBasicBlock() {
@@ -90,7 +91,7 @@ private cached module Cached {
     exists(Function f | f.getEntryPoint() = n)
     or
     // Okay to use successors_extended directly here
-    (not successors_extended(_,unresolveElement(n)) and not successors_extended(unresolveElement(n),_))
+    (not successors_extended(_,n) and not successors_extended(n,_))
     or
     reachable(n.getAPredecessor())
     or
@@ -142,6 +143,25 @@ private predicate loopConditionAlwaysUponEntry(ControlFlowNode loop, Expr condit
   )
 }
 
+/**
+ * An element that is convertible to `ControlFlowNode`. This class is similar
+ * to `ControlFlowNode` except that is has no member predicates apart from
+ * those inherited from `Locatable`.
+ *
+ * This class can be used as base class for classes that want to inherit the
+ * extent of `ControlFlowNode` without inheriting its public member predicates.
+ */
+class ControlFlowNodeBase extends Locatable, @cfgnode {
+}
+
+predicate truecond_base(ControlFlowNodeBase n1, ControlFlowNodeBase n2) {
+  truecond(unresolveElement(n1), unresolveElement(n2))
+}
+
+predicate falsecond_base(ControlFlowNodeBase n1, ControlFlowNodeBase n2) {
+  falsecond(unresolveElement(n1), unresolveElement(n2))
+}
+
 /**
  * An abstract class that can be extended to add additional edges to the
  * control-flow graph. Instances of this class correspond to the source nodes
@@ -158,20 +178,19 @@ private predicate loopConditionAlwaysUponEntry(ControlFlowNode loop, Expr condit
  * appear to be unreachable. See the documentation on `ControlFlowNode` for
  * more information about the control-flow graph.
  */
-abstract class AdditionalControlFlowEdge extends @cfgnode {
+abstract class AdditionalControlFlowEdge extends ControlFlowNodeBase {
   /** Gets a target node of this edge, where the source node is `this`. */
-  abstract ControlFlowNode getAnEdgeTarget();
-
-  string toString() { result = mkElement(this).(ControlFlowNode).toString() }
+  abstract ControlFlowNodeBase getAnEdgeTarget();
 }
 
 /**
  * Holds if there is a control-flow edge from `source` to `target` in either
  * the extractor-generated control-flow graph or in a subclass of
  * `AdditionalControlFlowEdge`. Use this relation instead of `successors`.
  */
-predicate successors_extended(@cfgnode source, @cfgnode target) {
-  successors(source, target)
+predicate successors_extended(
+    ControlFlowNodeBase source, ControlFlowNodeBase target) {
+  successors(unresolveElement(source), unresolveElement(target))
   or
-  source.(AdditionalControlFlowEdge).getAnEdgeTarget() = mkElement(target)
+  source.(AdditionalControlFlowEdge).getAnEdgeTarget() = target
 }

cpp/ql/src/semmle/code/cpp/controlflow/DefinitionsAndUses.qll

@@ -11,9 +11,9 @@ private import semmle.code.cpp.dataflow.EscapesTree
  */
 predicate definitionUsePair(SemanticStackVariable var, Expr def, Expr use) {
   exists(Use u |
-    mkElement(u) = use
+    u = use
     and
-    unresolveElement(def).(Def).reaches(true, var, u)
+    def.(Def).reaches(true, var, u)
     and
     u.getVariable(false) = var
   )
@@ -24,7 +24,7 @@ predicate definitionUsePair(SemanticStackVariable var, Expr def, Expr use) {
  * is a definition or use, without crossing definitions of the same variable.
  */
 predicate definitionReaches(Expr def, Expr node) {
-  unresolveElement(def).(Def).reaches(true, _, (DefOrUse)unresolveElement(node))
+  def.(Def).reaches(true, _, (DefOrUse)node)
 }
 
 private predicate hasAddressOfAccess(SemanticStackVariable var) {
@@ -61,9 +61,9 @@ predicate useUsePair(SemanticStackVariable var, Expr first, Expr second) {
   not definition(var, first)
   and
   exists(Use u |
-    mkElement(u) = second
+    u = second
     and
-    unresolveElement(first).(Use).reaches(false, var, u)
+    first.(Use).reaches(false, var, u)
     and
     u.getVariable(false) = var
   )
@@ -76,19 +76,19 @@ predicate useUsePair(SemanticStackVariable var, Expr first, Expr second) {
 predicate parameterUsePair(Parameter p, VariableAccess va) {
   not parameterIsOverwritten(_, p) and va.getTarget() = p
   or
-  exists(ParameterDef pd | pd.reaches(true, p, (Use)unresolveElement(va)))
+  exists(ParameterDef pd | pd.reaches(true, p, (Use)va))
 }
 
 /**
  * Utility class: A definition or use of a stack variable.
  */
 library
-class DefOrUse extends @cfgnode {
+class DefOrUse extends ControlFlowNodeBase {
   DefOrUse() {
     // Uninstantiated templates are purely syntax, and only on instantiation
     // will they be complete with information about types, conversions, call
     // targets, etc.
-    not mkElement(this).isFromUninstantiatedTemplate(_)
+    not this.isFromUninstantiatedTemplate(_)
   }
 
   /**
@@ -104,7 +104,7 @@ class DefOrUse extends @cfgnode {
   pragma[noinline]
   private predicate reaches_helper(boolean isDef, SemanticStackVariable v, BasicBlock bb, int i) {
     getVariable(isDef) = v and
-    bb.getNode(i) = mkElement(this)
+    bb.getNode(i) = this
   }
 
   /**
@@ -140,24 +140,21 @@ class DefOrUse extends @cfgnode {
     exists(BasicBlock bb, int i |
       getVariable(isDef) = v
       and
-      bb.getNode(i) = mkElement(this)
+      bb.getNode(i) = this
       and
       j = min(int k | bbBarrierAt(bb, k, v, _) and k > i)
     )
   }
-
-  /** Gets a textual representation of this element. */
-  string toString() { result = "DefOrUse" }
 }
 
 library
 class Def extends DefOrUse {
   Def() {
-    definition(_, mkElement(this))
+    definition(_, this)
   }
 
   override SemanticStackVariable getVariable(boolean isDef) {
-    definition(result, mkElement(this)) and isDef = true
+    definition(result, this) and isDef = true
   }
 }
 
@@ -172,11 +169,11 @@ class ParameterDef extends DefOrUse {
   ParameterDef() {
     // Optimization: parameters that are not overwritten do not require
     // reachability analysis
-    exists(Function f | parameterIsOverwritten(f, _) | mkElement(this) = f.getEntryPoint())
+    exists(Function f | parameterIsOverwritten(f, _) | this = f.getEntryPoint())
   }
 
   override SemanticStackVariable getVariable(boolean isDef) {
-    exists(Function f | parameterIsOverwritten(f, result) | mkElement(this) = f.getEntryPoint())
+    exists(Function f | parameterIsOverwritten(f, result) | this = f.getEntryPoint())
     and
     isDef = true
   }
@@ -185,22 +182,22 @@ class ParameterDef extends DefOrUse {
 library
 class Use extends DefOrUse {
   Use() {
-    useOfVar(_, mkElement(this))
+    useOfVar(_, this)
   }
 
   override SemanticStackVariable getVariable(boolean isDef) {
-    useOfVar(result, mkElement(this)) and isDef = false
+    useOfVar(result, this) and isDef = false
   }
 }
 
 private predicate bbUseAt(BasicBlock bb, int i, SemanticStackVariable v, Use use) {
-  bb.getNode(i) = mkElement(use)
+  bb.getNode(i) = use
   and
   use.getVariable(false) = v
 }
 
 private predicate bbDefAt(BasicBlock bb, int i, SemanticStackVariable v, Def def) {
-  bb.getNode(i) = mkElement(def)
+  bb.getNode(i) = def
   and
   def.getVariable(true) = v
 }
@@ -403,8 +400,8 @@ predicate useOfVarActual(SemanticStackVariable v, VariableAccess use) {
 private predicate excludeReachesFunction(Function f) {
   exists(int defOrUses |
     defOrUses =
-      count(Def def | mkElement(def).(ControlFlowNode).getControlFlowScope() = f) +
-      count(Use use | mkElement(use).(ControlFlowNode).getControlFlowScope() = f) and
+      count(Def def | def.(ControlFlowNode).getControlFlowScope() = f) +
+      count(Use use | use.(ControlFlowNode).getControlFlowScope() = f) and
     defOrUses >= 13000
   )
 }

cpp/ql/src/semmle/code/cpp/controlflow/Dominance.qll

@@ -28,11 +28,6 @@ predicate functionEntry(ControlFlowNode entry) {
     and not hasMultiScopeNode(function))
 }
 
-/** Holds if `entry` is the entry point of a function. */
-predicate functionEntryBB(@cfgnode entry) {
-  functionEntry(mkElement(entry))
-}
-
 /**
  * Holds if `dest` is an immediate successor of `src` in the control-flow graph.
  */
@@ -70,7 +65,7 @@ predicate dominates(ControlFlowNode dominator, ControlFlowNode node) {
  * Holds if `dominator` is an immediate dominator of `node` in the control-flow
  * graph of basic blocks.
  */
-predicate bbIDominates(BasicBlock dom, BasicBlock node) = idominance(functionEntryBB/1, bb_successor/2)(_, dom, node)
+predicate bbIDominates(BasicBlock dom, BasicBlock node) = idominance(functionEntry/1, bb_successor/2)(_, dom, node)
 
 /**
  * Holds if `dominator` is a strict dominator of `node` in the control-flow

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

@@ -93,12 +93,12 @@ class GuardCondition extends Expr {
         exists(BasicBlock thisblock
         | thisblock.contains(this)
         | exists(BasicBlock succ
-          | testIsTrue = true and mkElement(succ) = this.getATrueSuccessor() or
-            testIsTrue = false and mkElement(succ) = this.getAFalseSuccessor()
+          | testIsTrue = true and succ = this.getATrueSuccessor() or
+            testIsTrue = false and succ = this.getAFalseSuccessor()
           | bbDominates(succ, controlled) and
             forall(BasicBlock pred
             | pred.getASuccessor() = succ
-            | pred = thisblock or bbDominates(succ, pred) or not reachable(mkElement(pred)))))
+            | pred = thisblock or bbDominates(succ, pred) or not reachable(pred))))
     }
 }