Shared: Address review comments for shared basic block library

Author: paldepind

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

@@ -96,9 +96,9 @@ private module CfgInput implements CfgShared::InputSig<Location> {
     t instanceof ST::SuccessorTypes::ExitSuccessor
   }
 
-  predicate idOfAstNode(AstNode node, int id) { node.getId() = id }
+  int idOfAstNode(AstNode node) { result = node.getId() }
 
-  predicate idOfCfgScope(CfgScope node, int id) { idOfAstNode(node, id) }
+  int idOfCfgScope(CfgScope node) { result = idOfAstNode(node) }
 }
 
 private module CfgSplittingInput implements CfgShared::SplittingInputSig<Location, CfgInput> {

ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll

@@ -4,11 +4,9 @@ private import codeql.ruby.AST
 private import codeql.ruby.ast.internal.AST
 private import codeql.ruby.ast.internal.TreeSitter
 private import codeql.ruby.controlflow.ControlFlowGraph
-private import codeql.ruby.controlflow.ControlFlowGraph as Cfg
 private import internal.ControlFlowGraphImpl as CfgImpl
 private import CfgNodes
 private import SuccessorTypes
-private import codeql.controlflow.BasicBlock as BB
 private import CfgImpl::BasicBlocks as BasicBlocksImpl
 
 /**

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -65,9 +65,9 @@ private module CfgInput implements CfgShared::InputSig<Location> {
 
   private predicate idOf(Ruby::AstNode node, int id) = equivalenceRelation(id/2)(node, id)
 
-  predicate idOfAstNode(AstNode node, int id) { idOf(AstInternal::toGeneratedInclSynth(node), id) }
+  int idOfAstNode(AstNode node) { idOf(AstInternal::toGeneratedInclSynth(node), result) }
 
-  predicate idOfCfgScope(CfgScope node, int id) { idOfAstNode(node, id) }
+  int idOfCfgScope(CfgScope node) { result = idOfAstNode(node) }
 }
 
 private module CfgSplittingInput implements CfgShared::SplittingInputSig<Location, CfgInput> {

rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll

@@ -56,11 +56,9 @@ private module CfgInput implements InputSig<Location> {
   private predicate idOfDbAstNode(Raw::AstNode x, int y) = equivalenceRelation(id/2)(x, y)
 
   // TODO: does not work if fresh ipa entities (`ipa: on:`) turn out to be first of the block
-  predicate idOfAstNode(AstNode node, int id) {
-    idOfDbAstNode(Synth::convertAstNodeToRaw(node), id)
-  }
+  int idOfAstNode(AstNode node) { idOfDbAstNode(Synth::convertAstNodeToRaw(node), result) }
 
-  predicate idOfCfgScope(CfgScope node, int id) { idOfAstNode(node, id) }
+  int idOfCfgScope(CfgScope node) { result = idOfAstNode(node) }
 }
 
 private module CfgSplittingInput implements SplittingInputSig<Location, CfgInput> {

shared/controlflow/codeql/controlflow/BasicBlock.qll

@@ -1,13 +1,15 @@
 /**
- * This modules provides an implementation of a basic block class based based
- * on a control flow graph implementation.
+ * This modules provides an implementation of a basic block class based on a
+ * control flow graph implementation.
  *
  * INTERNAL use only. This is an experimental API subject to change without
  * notice.
  */
 
+private import codeql.util.Location
+
 /** Provides the language-specific input specification. */
-signature module InputSig {
+signature module InputSig<LocationSig Location> {
   class SuccessorType;
 
   /** Hold if `t` represents a conditional successor type. */
@@ -16,21 +18,31 @@ signature module InputSig {
   /** The class of control flow nodes. */
   class Node {
     string toString();
+
+    /** Gets the location of this control flow node. */
+    Location getLocation();
   }
 
+  /** Gets an immediate successor of this node. */
   Node nodeGetASuccessor(Node node, SuccessorType t);
 
-  /** Holds if `node` is the beginning of an entry basic block. */
-  predicate nodeIsEntry(Node node);
+  /**
+   * Holds if `node` represents an entry node to be used when calculating
+   * dominance.
+   */
+  predicate nodeIsDominanceEntry(Node node);
 
-  /** Holds if `node` is the beginning of an entry basic block. */
-  predicate nodeIsExit(Node node);
+  /**
+   * Holds if `node` represents an exit node to be used when calculating
+   * post dominance.
+   */
+  predicate nodeIsPostDominanceExit(Node node);
 }
 
 /**
  * Provides a basic block construction on top of a control flow graph.
  */
-module Make<InputSig Input> {
+module Make<LocationSig Location, InputSig<Location> Input> {
   private import Input
 
   final class BasicBlock = BasicBlockImpl;
@@ -42,13 +54,17 @@ module Make<InputSig Input> {
   /** Holds if this node has more than one predecessor. */
   private predicate nodeIsJoin(Node node) { strictcount(nodeGetAPredecessor(node, _)) > 1 }
 
+  /** Holds if this node has more than one successor. */
   private predicate nodeIsBranch(Node node) { strictcount(nodeGetASuccessor(node, _)) > 1 }
 
   /**
    * A basic block, that is, a maximal straight-line sequence of control flow nodes
    * without branches or joins.
    */
   private class BasicBlockImpl extends TBasicBlockStart {
+    /** Gets the location of this basic block. */
+    Location getLocation() { result = this.getFirstNode().getLocation() }
+
     /** Gets an immediate successor of this basic block, if any. */
     BasicBlock getASuccessor() { result = this.getASuccessor(_) }
 
@@ -64,6 +80,7 @@ module Make<InputSig Input> {
     BasicBlock getAPredecessor(SuccessorType t) { result.getASuccessor(t) = this }
 
     /** Gets the control flow node at a specific (zero-indexed) position in this basic block. */
+    cached
     Node getNode(int pos) { bbIndex(this.getFirstNode(), result, pos) }
 
     /** Gets a control flow node in this basic block. */
@@ -166,52 +183,6 @@ module Make<InputSig Input> {
     cached
     newtype TBasicBlock = TBasicBlockStart(Node cfn) { startsBB(cfn) }
 
-    /** Holds if `cfn` starts a new basic block. */
-    private predicate startsBB(Node cfn) {
-      not exists(nodeGetAPredecessor(cfn, _)) and exists(nodeGetASuccessor(cfn, _))
-      or
-      nodeIsJoin(cfn)
-      or
-      nodeIsBranch(nodeGetAPredecessor(cfn, _))
-      or
-      // In cases such as
-      //
-      // ```rb
-      // if x or y
-      //     foo
-      // else
-      //     bar
-      // ```
-      //
-      // we have a CFG that looks like
-      //
-      // x --false--> [false] x or y --false--> bar
-      // \                    |
-      //  --true--> y --false--
-      //            \
-      //             --true--> [true] x or y --true--> foo
-      //
-      // and we want to ensure that both `foo` and `bar` start a new basic block.
-      exists(nodeGetAPredecessor(cfn, any(SuccessorType s | successorTypeIsCondition(s))))
-    }
-
-    /**
-     * Holds if `succ` is a control flow successor of `pred` within
-     * the same basic block.
-     */
-    private predicate intraBBSucc(Node pred, Node succ) {
-      succ = nodeGetASuccessor(pred, _) and
-      not startsBB(succ)
-    }
-
-    /**
-     * Holds if `bbStart` is the first node in a basic block and `cfn` is the
-     * `i`th node in the same basic block.
-     */
-    cached
-    predicate bbIndex(Node bbStart, Node cfn, int i) =
-      shortestDistances(startsBB/1, intraBBSucc/2)(bbStart, cfn, i)
-
     /**
      * Holds if the first node of basic block `succ` is a control flow
      * successor of the last node of basic block `pred`.
@@ -227,7 +198,7 @@ module Make<InputSig Input> {
     private predicate predBB(BasicBlock succ, BasicBlock pred) { succBB(pred, succ) }
 
     /** Holds if `bb` is an exit basic block that represents normal exit. */
-    private predicate exitBB(BasicBlock bb) { nodeIsExit(bb.getANode()) }
+    private predicate exitBB(BasicBlock bb) { nodeIsPostDominanceExit(bb.getANode()) }
 
     /** Holds if `dom` is an immediate post-dominator of `bb`. */
     cached
@@ -237,6 +208,51 @@ module Make<InputSig Input> {
 
   private import Cached
 
+  /** Holds if `cfn` starts a new basic block. */
+  private predicate startsBB(Node cfn) {
+    not exists(nodeGetAPredecessor(cfn, _)) and exists(nodeGetASuccessor(cfn, _))
+    or
+    nodeIsJoin(cfn)
+    or
+    nodeIsBranch(nodeGetAPredecessor(cfn, _))
+    or
+    // In cases such as
+    //
+    // ```rb
+    // if x or y
+    //     foo
+    // else
+    //     bar
+    // ```
+    //
+    // we have a CFG that looks like
+    //
+    // x --false--> [false] x or y --false--> bar
+    // \                    |
+    //  --true--> y --false--
+    //            \
+    //             --true--> [true] x or y --true--> foo
+    //
+    // and we want to ensure that both `foo` and `bar` start a new basic block.
+    exists(nodeGetAPredecessor(cfn, any(SuccessorType s | successorTypeIsCondition(s))))
+  }
+
+  /**
+   * Holds if `succ` is a control flow successor of `pred` within
+   * the same basic block.
+   */
+  predicate intraBBSucc(Node pred, Node succ) {
+    succ = nodeGetASuccessor(pred, _) and
+    not startsBB(succ)
+  }
+
+  /**
+   * Holds if `bbStart` is the first node in a basic block and `cfn` is the
+   * `i`th node in the same basic block.
+   */
+  private predicate bbIndex(Node bbStart, Node cfn, int i) =
+    shortestDistances(startsBB/1, intraBBSucc/2)(bbStart, cfn, i)
+
   /** Holds if `bb` is an entry basic block. */
-  private predicate entryBB(BasicBlock bb) { nodeIsEntry(bb.getFirstNode()) }
+  private predicate entryBB(BasicBlock bb) { nodeIsDominanceEntry(bb.getFirstNode()) }
 }

shared/controlflow/codeql/controlflow/Cfg.qll

@@ -83,14 +83,14 @@ signature module InputSig<LocationSig Location> {
    * basic block.
    */
   bindingset[node]
-  default predicate idOfAstNode(AstNode node, int id) { none() }
+  int idOfAstNode(AstNode node);
 
   /**
    * Gets an `id` of `scope`. This is used to order the predecessors of a join
    * basic block.
    */
   bindingset[scope]
-  default predicate idOfCfgScope(CfgScope scope, int id) { none() }
+  int idOfCfgScope(CfgScope scope);
 }
 
 /** Provides input needed for CFG splitting. */
@@ -1537,7 +1537,7 @@ module MakeWithSplitting<
 
     private class NodeAlias = Node;
 
-    private module BasicBlockInputSig implements BB::InputSig {
+    private module BasicBlockInputSig implements BB::InputSig<Location> {
       class SuccessorType = Input::SuccessorType;
 
       predicate successorTypeIsCondition = Input::successorTypeIsCondition/1;
@@ -1546,30 +1546,26 @@ module MakeWithSplitting<
 
       Node nodeGetASuccessor(Node node, SuccessorType t) { result = node.getASuccessor(t) }
 
-      predicate nodeIsEntry(Node node) { node instanceof EntryNode }
+      predicate nodeIsDominanceEntry(Node node) { node instanceof EntryNode }
 
-      predicate nodeIsExit(Node node) { node.(AnnotatedExitNode).isNormal() }
+      predicate nodeIsPostDominanceExit(Node node) { node.(AnnotatedExitNode).isNormal() }
     }
 
-    private module BasicBlockImpl = BB::Make<BasicBlockInputSig>;
+    private module BasicBlockImpl = BB::Make<Location, BasicBlockInputSig>;
 
     /**
      * A basic block, that is, a maximal straight-line sequence of control flow nodes
      * without branches or joins.
      */
     final class BasicBlock extends BasicBlockImpl::BasicBlock {
-      // We extend `BasicBlockImpl::BasicBlock` to add the `getScope` and
-      // `getLocation`.
+      // We extend `BasicBlockImpl::BasicBlock` to add the `getScope`.
       /** Gets the scope of this basic block. */
       CfgScope getScope() {
         if this instanceof EntryBasicBlock
         then result = this.getFirstNode().getScope()
         else result = this.getAPredecessor().getScope()
       }
 
-      /** Gets the location of this basic block. */
-      Location getLocation() { result = this.getFirstNode().getLocation() }
-
       /** Gets an immediate successor of this basic block, if any. */
       BasicBlock getASuccessor() { result = super.getASuccessor() }
 
@@ -1678,10 +1674,13 @@ module MakeWithSplitting<
     }
 
     private module JoinBlockPredecessors {
-      int getId(JoinPredecessorBasicBlock jbp) {
-        idOfAstNode(jbp.getFirstNode().(AstCfgNode).getAstNode(), result)
+      predicate hasIdAndKind(JoinPredecessorBasicBlock jbp, int id, int kind) {
+        id = idOfCfgScope(jbp.(EntryBasicBlock).getScope()) and
+        kind = 0
         or
-        idOfCfgScope(jbp.(EntryBasicBlock).getScope(), result)
+        not jbp instanceof EntryBasicBlock and
+        id = idOfAstNode(jbp.getFirstNode().(AstCfgNode).getAstNode()) and
+        kind = 1
       }
 
       string getSplitString(JoinPredecessorBasicBlock jbp) {
@@ -1699,10 +1698,10 @@ module MakeWithSplitting<
     cached
     JoinPredecessorBasicBlock getJoinBlockPredecessor(JoinBasicBlock jb, int i) {
       result =
-        rank[i + 1](JoinPredecessorBasicBlock jbp |
-          jbp = jb.getAPredecessor()
+        rank[i + 1](JoinPredecessorBasicBlock jbp, int id, int kind |
+          jbp = jb.getAPredecessor() and JoinBlockPredecessors::hasIdAndKind(jbp, id, kind)
         |
-          jbp order by JoinBlockPredecessors::getId(jbp), JoinBlockPredecessors::getSplitString(jbp)
+          jbp order by id, kind, JoinBlockPredecessors::getSplitString(jbp)
         )
     }
 

swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImplSpecific.qll

@@ -106,9 +106,9 @@ module CfgInput implements InputSig<Location> {
     result = n.(FuncDeclElement).getAst()
   }
 
-  predicate idOfAstNode(AstNode node, int id) { idOf(projectToAst(node), id) }
+  int idOfAstNode(AstNode node) { idOf(projectToAst(node), result) }
 
-  predicate idOfCfgScope(CfgScope node, int id) { idOf(node, id) }
+  int idOfCfgScope(CfgScope node) { idOf(node, result) }
 }
 
 private module CfgSplittingInput implements SplittingInputSig<Location, CfgInput> {