From 17ee3f555c393be747b9464ffcd2d0e0d72e3f31 Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jonas@semmle.com>
Date: Mon, 26 Aug 2019 16:01:36 +0200
Subject: [PATCH 1/7] C++: Sync the two copies of SubBasicBlocks.qll

These files are now added to `identical-files.json` so they will remain
in sync.
---
 config/identical-files.json                   |  4 ++
 .../code/cpp/controlflow/SubBasicBlocks.qll   | 52 +++++++++++++------
 .../cpp/dataflow/internal/SubBasicBlocks.qll  | 14 ++---
 3 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/config/identical-files.json b/config/identical-files.json
index 08c3554de5a..3ab934b4c02 100644
--- a/config/identical-files.json
+++ b/config/identical-files.json
@@ -36,6 +36,10 @@
     "java/ql/src/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
     "java/ql/src/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll"
   ],
+  "C++ SubBasicBlocks": [
+    "cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll",
+    "cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll"
+  ],
   "IR Instruction": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll",
diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index 85ac548dc4a..4ca7dafe939 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -1,7 +1,11 @@
+// NOTE: There are two copies of this file, and they must be kept identical:
+// - semmle/code/cpp/controlflow/SubBasicBlocks.qll
+// - semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
 //
-// NOTE: Maintain this file in synchrony with
-// semmlecode-cpp-queries/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
-//
+// The second one is a private copy of the `SubBasicBlocks` library for
+// internal use by the data flow library. Having an extra copy prevents
+// non-monotonic recursion errors in queries that use both the data flow
+// library and the `SubBasicBlocks` library.
 
 /**
  * Provides the `SubBasicBlock` class, used for partitioning basic blocks in
@@ -71,14 +75,18 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * `bb`, where `bb` is equal to `getBasicBlock()`.
    */
   int getPosInBasicBlock(BasicBlock bb) {
-    exists(int nodePos, int rnk |
-      bb = this.(ControlFlowNode).getBasicBlock() and
-      this = bb.getNode(nodePos) and
-      nodePos = rank[rnk](int i | exists(SubBasicBlock n | n = bb.getNode(i))) and
+    exists(int thisIndexInBB, int rnk |
+      thisIndexInBB = this.getIndexInBasicBlock(bb) and
+      thisIndexInBB = rank[rnk](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb)) and
       result = rnk - 1
     )
   }
 
+  pragma[noinline]
+  private int getIndexInBasicBlock(BasicBlock bb) {
+    this = bb.getNode(result)
+  }
+
   /** Gets a successor in the control-flow graph of `SubBasicBlock`s. */
   SubBasicBlock getASuccessor() {
     this.lastInBB() and
@@ -94,16 +102,22 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * start from 0, and the node at position 0 always exists and compares equal
    * to `this`.
    */
+  pragma[nomagic]
   ControlFlowNode getNode(int pos) {
-    exists(BasicBlock bb | bb = this.getBasicBlock() |
-      exists(int thisPos | this = bb.getNode(thisPos) |
-        result = bb.getNode(thisPos + pos) and
-        pos >= 0 and
-        pos < this.getNumberOfNodes()
+    exists(BasicBlock bb |
+      exists(int outerPos |
+        result = bb.getNode(outerPos) and
+        pos = outerPosToInnerPos(bb, outerPos)
       )
     )
   }
 
+  pragma[nomagic]
+  private int outerPosToInnerPos(BasicBlock bb, int posInBB) {
+    posInBB = result + this.getIndexInBasicBlock(bb) and
+    result = [ 0 .. this.getNumberOfNodes() - 1 ]
+  }
+
   /** Gets a control-flow node in this `SubBasicBlock`. */
   ControlFlowNode getANode() {
     result = this.getNode(_)
@@ -143,14 +157,20 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * Gets the number of control-flow nodes in this `SubBasicBlock`. There is
    * always at least one.
    */
+  pragma[noopt]
   int getNumberOfNodes() {
     exists(BasicBlock bb | bb = this.getBasicBlock() |
       exists(int thisPos | this = bb.getNode(thisPos) |
-        this.lastInBB() and
-        result = bb.length() - thisPos
+        exists(int bbLength |
+          this.lastInBB() and
+          bbLength = bb.length() and
+          result = bbLength - thisPos
+        )
         or
-        exists(SubBasicBlock succ, int succPos |
-          succ.getPosInBasicBlock(bb) = this.getPosInBasicBlock(bb) + 1 and
+        exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
+          thisRank = this.getPosInBasicBlock(bb) and
+          succRank = thisRank + 1 and
+          succRank = succ.getPosInBasicBlock(bb) and
           bb.getNode(succPos) = succ and
           result = succPos - thisPos
         )
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index e6ce8c4ae9a..4ca7dafe939 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -1,11 +1,11 @@
+// NOTE: There are two copies of this file, and they must be kept identical:
+// - semmle/code/cpp/controlflow/SubBasicBlocks.qll
+// - semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
 //
-// NOTE: Maintain this file in synchrony with
-// semmlecode-cpp-queries/semmle/code/cpp/controlflow/SubBasicBlocks.qll
-//
-// This is a private copy of the `SubBasicBlocks` library for internal use by
-// the data flow library. Having an extra copy can prevent non-monotonic
-// recursion errors in queries that use both the data flow library and the
-// `SubBasicBlocks` library.
+// The second one is a private copy of the `SubBasicBlocks` library for
+// internal use by the data flow library. Having an extra copy prevents
+// non-monotonic recursion errors in queries that use both the data flow
+// library and the `SubBasicBlocks` library.
 
 /**
  * Provides the `SubBasicBlock` class, used for partitioning basic blocks in

From 2b94bb9edabcba35d3563b99e39c85dad90a6316 Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jonas@semmle.com>
Date: Mon, 26 Aug 2019 16:13:08 +0200
Subject: [PATCH 2/7] C++: Use the word "rank" to denote a rank

Previously, the word "position" was used ambiguously in this library.
---
 .../code/cpp/controlflow/SubBasicBlocks.qll   | 21 +++++++++----------
 .../cpp/dataflow/internal/SubBasicBlocks.qll  | 21 +++++++++----------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index 4ca7dafe939..a3a28bf1ae6 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -57,7 +57,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * predecessors.
    */
   predicate firstInBB() {
-    exists(BasicBlock bb | this.getPosInBasicBlock(bb) = 0)
+    exists(BasicBlock bb | this.getRankInBasicBlock(bb) = 1)
   }
 
   /**
@@ -66,19 +66,18 @@ class SubBasicBlock extends ControlFlowNodeBase {
    */
   predicate lastInBB() {
     exists(BasicBlock bb |
-      this.getPosInBasicBlock(bb) = countSubBasicBlocksInBasicBlock(bb) - 1
+      this.getRankInBasicBlock(bb) = countSubBasicBlocksInBasicBlock(bb)
     )
   }
 
   /**
-   * Gets the position of this `SubBasicBlock` in its containing basic block
-   * `bb`, where `bb` is equal to `getBasicBlock()`.
+   * Gets the rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
+   * its containing basic block `bb`, where `bb` is equal to `getBasicBlock()`.
    */
-  int getPosInBasicBlock(BasicBlock bb) {
-    exists(int thisIndexInBB, int rnk |
+  int getRankInBasicBlock(BasicBlock bb) {
+    exists(int thisIndexInBB |
       thisIndexInBB = this.getIndexInBasicBlock(bb) and
-      thisIndexInBB = rank[rnk](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb)) and
-      result = rnk - 1
+      thisIndexInBB = rank[result](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb))
     )
   }
 
@@ -93,7 +92,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
     result = this.getBasicBlock().getASuccessor()
     or
     exists(BasicBlock bb |
-      result.getPosInBasicBlock(bb) = this.getPosInBasicBlock(bb) + 1
+      result.getRankInBasicBlock(bb) = this.getRankInBasicBlock(bb) + 1
     )
   }
 
@@ -168,9 +167,9 @@ class SubBasicBlock extends ControlFlowNodeBase {
         )
         or
         exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
-          thisRank = this.getPosInBasicBlock(bb) and
+          thisRank = this.getRankInBasicBlock(bb) and
           succRank = thisRank + 1 and
-          succRank = succ.getPosInBasicBlock(bb) and
+          succRank = succ.getRankInBasicBlock(bb) and
           bb.getNode(succPos) = succ and
           result = succPos - thisPos
         )
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index 4ca7dafe939..a3a28bf1ae6 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -57,7 +57,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * predecessors.
    */
   predicate firstInBB() {
-    exists(BasicBlock bb | this.getPosInBasicBlock(bb) = 0)
+    exists(BasicBlock bb | this.getRankInBasicBlock(bb) = 1)
   }
 
   /**
@@ -66,19 +66,18 @@ class SubBasicBlock extends ControlFlowNodeBase {
    */
   predicate lastInBB() {
     exists(BasicBlock bb |
-      this.getPosInBasicBlock(bb) = countSubBasicBlocksInBasicBlock(bb) - 1
+      this.getRankInBasicBlock(bb) = countSubBasicBlocksInBasicBlock(bb)
     )
   }
 
   /**
-   * Gets the position of this `SubBasicBlock` in its containing basic block
-   * `bb`, where `bb` is equal to `getBasicBlock()`.
+   * Gets the rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
+   * its containing basic block `bb`, where `bb` is equal to `getBasicBlock()`.
    */
-  int getPosInBasicBlock(BasicBlock bb) {
-    exists(int thisIndexInBB, int rnk |
+  int getRankInBasicBlock(BasicBlock bb) {
+    exists(int thisIndexInBB |
       thisIndexInBB = this.getIndexInBasicBlock(bb) and
-      thisIndexInBB = rank[rnk](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb)) and
-      result = rnk - 1
+      thisIndexInBB = rank[result](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb))
     )
   }
 
@@ -93,7 +92,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
     result = this.getBasicBlock().getASuccessor()
     or
     exists(BasicBlock bb |
-      result.getPosInBasicBlock(bb) = this.getPosInBasicBlock(bb) + 1
+      result.getRankInBasicBlock(bb) = this.getRankInBasicBlock(bb) + 1
     )
   }
 
@@ -168,9 +167,9 @@ class SubBasicBlock extends ControlFlowNodeBase {
         )
         or
         exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
-          thisRank = this.getPosInBasicBlock(bb) and
+          thisRank = this.getRankInBasicBlock(bb) and
           succRank = thisRank + 1 and
-          succRank = succ.getPosInBasicBlock(bb) and
+          succRank = succ.getRankInBasicBlock(bb) and
           bb.getNode(succPos) = succ and
           result = succPos - thisPos
         )

From 92c354e8e8757f579596f5ac78e9eda58c80a085 Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jbj@knef.dk>
Date: Mon, 26 Aug 2019 21:11:42 +0200
Subject: [PATCH 3/7] C++: Simplify SubBasicBlock::getNumberOfNodes

---
 .../code/cpp/controlflow/SubBasicBlocks.qll   | 21 ++++---------------
 .../cpp/dataflow/internal/SubBasicBlocks.qll  | 21 ++++---------------
 2 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index a3a28bf1ae6..c1f6f80067b 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -156,24 +156,11 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * Gets the number of control-flow nodes in this `SubBasicBlock`. There is
    * always at least one.
    */
-  pragma[noopt]
   int getNumberOfNodes() {
-    exists(BasicBlock bb | bb = this.getBasicBlock() |
-      exists(int thisPos | this = bb.getNode(thisPos) |
-        exists(int bbLength |
-          this.lastInBB() and
-          bbLength = bb.length() and
-          result = bbLength - thisPos
-        )
-        or
-        exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
-          thisRank = this.getRankInBasicBlock(bb) and
-          succRank = thisRank + 1 and
-          succRank = succ.getRankInBasicBlock(bb) and
-          bb.getNode(succPos) = succ and
-          result = succPos - thisPos
-        )
-      )
+    exists(BasicBlock bb |
+      if this.lastInBB()
+      then result = bb.length() - this.getIndexInBasicBlock(bb)
+      else result = this.getASuccessor().getIndexInBasicBlock(bb) - this.getIndexInBasicBlock(bb)
     )
   }
 
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index a3a28bf1ae6..c1f6f80067b 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -156,24 +156,11 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * Gets the number of control-flow nodes in this `SubBasicBlock`. There is
    * always at least one.
    */
-  pragma[noopt]
   int getNumberOfNodes() {
-    exists(BasicBlock bb | bb = this.getBasicBlock() |
-      exists(int thisPos | this = bb.getNode(thisPos) |
-        exists(int bbLength |
-          this.lastInBB() and
-          bbLength = bb.length() and
-          result = bbLength - thisPos
-        )
-        or
-        exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
-          thisRank = this.getRankInBasicBlock(bb) and
-          succRank = thisRank + 1 and
-          succRank = succ.getRankInBasicBlock(bb) and
-          bb.getNode(succPos) = succ and
-          result = succPos - thisPos
-        )
-      )
+    exists(BasicBlock bb |
+      if this.lastInBB()
+      then result = bb.length() - this.getIndexInBasicBlock(bb)
+      else result = this.getASuccessor().getIndexInBasicBlock(bb) - this.getIndexInBasicBlock(bb)
     )
   }
 

From 2332dada1a06b28c400687e5f54c51272bd90184 Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jbj@knef.dk>
Date: Mon, 26 Aug 2019 21:29:20 +0200
Subject: [PATCH 4/7] C++: s/pos/index/ in SubBasicBlocks

---
 .../code/cpp/controlflow/SubBasicBlocks.qll   | 20 +++++++++++--------
 .../cpp/dataflow/internal/SubBasicBlocks.qll  | 20 +++++++++++--------
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index c1f6f80067b..caed3df15bd 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -97,23 +97,27 @@ class SubBasicBlock extends ControlFlowNodeBase {
   }
 
   /**
-   * Gets the `pos`th control-flow node in this `SubBasicBlock`. Positions
-   * start from 0, and the node at position 0 always exists and compares equal
+   * Gets the `index`th control-flow node in this `SubBasicBlock`. Indexes
+   * start from 0, and the node at index 0 always exists and compares equal
    * to `this`.
    */
   pragma[nomagic]
-  ControlFlowNode getNode(int pos) {
+  ControlFlowNode getNode(int index) {
     exists(BasicBlock bb |
-      exists(int outerPos |
-        result = bb.getNode(outerPos) and
-        pos = outerPosToInnerPos(bb, outerPos)
+      exists(int outerIndex |
+        result = bb.getNode(outerIndex) and
+        index = outerToInnerIndex(bb, outerIndex)
       )
     )
   }
 
+  /**
+   * Gets the index of the node in this `SubBasicBlock` that has `indexInBB` in
+   * `bb`, where `bb` is equal to `getBasicBlock()`.
+   */
   pragma[nomagic]
-  private int outerPosToInnerPos(BasicBlock bb, int posInBB) {
-    posInBB = result + this.getIndexInBasicBlock(bb) and
+  private int outerToInnerIndex(BasicBlock bb, int indexInBB) {
+    indexInBB = result + this.getIndexInBasicBlock(bb) and
     result = [ 0 .. this.getNumberOfNodes() - 1 ]
   }
 
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index c1f6f80067b..caed3df15bd 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -97,23 +97,27 @@ class SubBasicBlock extends ControlFlowNodeBase {
   }
 
   /**
-   * Gets the `pos`th control-flow node in this `SubBasicBlock`. Positions
-   * start from 0, and the node at position 0 always exists and compares equal
+   * Gets the `index`th control-flow node in this `SubBasicBlock`. Indexes
+   * start from 0, and the node at index 0 always exists and compares equal
    * to `this`.
    */
   pragma[nomagic]
-  ControlFlowNode getNode(int pos) {
+  ControlFlowNode getNode(int index) {
     exists(BasicBlock bb |
-      exists(int outerPos |
-        result = bb.getNode(outerPos) and
-        pos = outerPosToInnerPos(bb, outerPos)
+      exists(int outerIndex |
+        result = bb.getNode(outerIndex) and
+        index = outerToInnerIndex(bb, outerIndex)
       )
     )
   }
 
+  /**
+   * Gets the index of the node in this `SubBasicBlock` that has `indexInBB` in
+   * `bb`, where `bb` is equal to `getBasicBlock()`.
+   */
   pragma[nomagic]
-  private int outerPosToInnerPos(BasicBlock bb, int posInBB) {
-    posInBB = result + this.getIndexInBasicBlock(bb) and
+  private int outerToInnerIndex(BasicBlock bb, int indexInBB) {
+    indexInBB = result + this.getIndexInBasicBlock(bb) and
     result = [ 0 .. this.getNumberOfNodes() - 1 ]
   }
 

From 4cae5917cbbac8a8fed073e832440e08709e9e6e Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jbj@knef.dk>
Date: Mon, 26 Aug 2019 21:48:18 +0200
Subject: [PATCH 5/7] C++: Remove one nomagic and explain the other

---
 cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll    | 5 ++++-
 .../src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index caed3df15bd..2d3434d1639 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -101,7 +101,6 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * start from 0, and the node at index 0 always exists and compares equal
    * to `this`.
    */
-  pragma[nomagic]
   ControlFlowNode getNode(int index) {
     exists(BasicBlock bb |
       exists(int outerIndex |
@@ -115,6 +114,10 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * Gets the index of the node in this `SubBasicBlock` that has `indexInBB` in
    * `bb`, where `bb` is equal to `getBasicBlock()`.
    */
+  // This predicate is factored out of `getNode` to ensure a good join order.
+  // It's sensitive to bad magic, so it has `pragma[nomagic]` on it. For
+  // example, it can get very slow if `getNode` is pragma[nomagic], which could
+  // mean it might get very slow if `getNode` is used in the wrong context.
   pragma[nomagic]
   private int outerToInnerIndex(BasicBlock bb, int indexInBB) {
     indexInBB = result + this.getIndexInBasicBlock(bb) and
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index caed3df15bd..2d3434d1639 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -101,7 +101,6 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * start from 0, and the node at index 0 always exists and compares equal
    * to `this`.
    */
-  pragma[nomagic]
   ControlFlowNode getNode(int index) {
     exists(BasicBlock bb |
       exists(int outerIndex |
@@ -115,6 +114,10 @@ class SubBasicBlock extends ControlFlowNodeBase {
    * Gets the index of the node in this `SubBasicBlock` that has `indexInBB` in
    * `bb`, where `bb` is equal to `getBasicBlock()`.
    */
+  // This predicate is factored out of `getNode` to ensure a good join order.
+  // It's sensitive to bad magic, so it has `pragma[nomagic]` on it. For
+  // example, it can get very slow if `getNode` is pragma[nomagic], which could
+  // mean it might get very slow if `getNode` is used in the wrong context.
   pragma[nomagic]
   private int outerToInnerIndex(BasicBlock bb, int indexInBB) {
     indexInBB = result + this.getIndexInBasicBlock(bb) and

From b98d6379e9f7c4ff9e5d66ae6bee97066738c3f6 Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jonas@semmle.com>
Date: Wed, 28 Aug 2019 08:29:06 +0200
Subject: [PATCH 6/7] C++: Restore and deprecate getPosInBasicBlock

This predicte was still used in a test, so it might be used in external
code too.
---
 .../semmle/code/cpp/controlflow/SubBasicBlocks.qll    | 11 ++++++++++-
 .../code/cpp/dataflow/internal/SubBasicBlocks.qll     | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
index 2d3434d1639..93ac33be2f0 100644
--- a/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/controlflow/SubBasicBlocks.qll
@@ -71,7 +71,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
   }
 
   /**
-   * Gets the rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
+   * Gets the (1-based) rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
    * its containing basic block `bb`, where `bb` is equal to `getBasicBlock()`.
    */
   int getRankInBasicBlock(BasicBlock bb) {
@@ -81,6 +81,15 @@ class SubBasicBlock extends ControlFlowNodeBase {
     )
   }
 
+  /**
+   * DEPRECATED: use `getRankInBasicBlock` instead. Note that this predicate
+   * returns a 0-based position, while `getRankInBasicBlock` returns a 1-based
+   * position.
+   */
+  deprecated int getPosInBasicBlock(BasicBlock bb) {
+    result = getRankInBasicBlock(bb) - 1
+  }
+
   pragma[noinline]
   private int getIndexInBasicBlock(BasicBlock bb) {
     this = bb.getNode(result)
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
index 2d3434d1639..93ac33be2f0 100644
--- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
+++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
@@ -71,7 +71,7 @@ class SubBasicBlock extends ControlFlowNodeBase {
   }
 
   /**
-   * Gets the rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
+   * Gets the (1-based) rank of this `SubBasicBlock` among the other `SubBasicBlock`s in
    * its containing basic block `bb`, where `bb` is equal to `getBasicBlock()`.
    */
   int getRankInBasicBlock(BasicBlock bb) {
@@ -81,6 +81,15 @@ class SubBasicBlock extends ControlFlowNodeBase {
     )
   }
 
+  /**
+   * DEPRECATED: use `getRankInBasicBlock` instead. Note that this predicate
+   * returns a 0-based position, while `getRankInBasicBlock` returns a 1-based
+   * position.
+   */
+  deprecated int getPosInBasicBlock(BasicBlock bb) {
+    result = getRankInBasicBlock(bb) - 1
+  }
+
   pragma[noinline]
   private int getIndexInBasicBlock(BasicBlock bb) {
     this = bb.getNode(result)

From 8c610e4f68641c387b42c5293e74d6b7900e91ec Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jonas@semmle.com>
Date: Wed, 28 Aug 2019 08:31:05 +0200
Subject: [PATCH 7/7] C++: Don't use deprecated interface in test

---
 cpp/ql/test/library-tests/sub_basic_blocks/sbb_test.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/test/library-tests/sub_basic_blocks/sbb_test.qll b/cpp/ql/test/library-tests/sub_basic_blocks/sbb_test.qll
index 185e4635853..3af0841cdd3 100644
--- a/cpp/ql/test/library-tests/sub_basic_blocks/sbb_test.qll
+++ b/cpp/ql/test/library-tests/sub_basic_blocks/sbb_test.qll
@@ -6,7 +6,7 @@ string subBasicBlockDebugInfo(SubBasicBlock sbb) {
     " [line " + sbb.getStart().getLocation().getStartLine() + "-" +
                 sbb.getEnd().getLocation().getEndLine() + ", " +
     sbb.getNumberOfNodes() + " nodes, " +
-    "pos " + sbb.getPosInBasicBlock(_) +
+    "pos " + (sbb.getRankInBasicBlock(_) - 1) +
     any(string s | if sbb.firstInBB() then s = " (first in BB)" else s = "") +
     any(string s | if sbb.lastInBB() then s = " (last in BB)" else s = "") +
     ", " +