C++: Remove infeasible edges to reachable blocks

The existing unreachable IR removal code only retargeted an infeasible edge to an `Unreached` instruction if the successor of the edge was an unreachable block. This is too conservative, because it doesn't remove an infeasible edge that targets a block that is still reachable via other paths. The trivial example of this is `do { } while (false);`, where the back edge is infeasible, but the body block is still reachable from the loop entry.

This change retargets all infeasible edges to `Unreached` instructions, regardless of the reachability of the successor block.

config/identical-files.json

@@ -94,5 +94,9 @@
   "C++ IR Dominance": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/reachability/Dominance.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/reachability/Dominance.qll"
+  ],
+  "C++ IR PrintDominance": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/reachability/PrintDominance.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/reachability/PrintDominance.qll"
   ]
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll

@@ -26,13 +26,9 @@ cached private module Cached {
       hasChiNode(_, oldInstruction)
     } or
     UnreachedTag(OldInstruction oldInstruction, EdgeKind kind) {
-      // We need an `Unreached` instruction for the destination of any edge whose predecessor
-      // instruction is reachable, but whose successor block is not. This should occur only for
-      // infeasible edges.
-      exists(OldIR::Instruction succInstruction |
-        succInstruction = oldInstruction.getSuccessor(kind) and
-        not succInstruction instanceof OldInstruction
-      )
+      // We need an `Unreached` instruction for the destination of each infeasible edge whose
+      // predecessor is reachable.
+      Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind)
     }
 
   cached class InstructionTagType extends TInstructionTag {
@@ -213,7 +209,7 @@ cached private module Cached {
     exists(Alias::VirtualVariable vvar, OldBlock phiBlock,
         OldBlock defBlock, int defRank, int defIndex, OldBlock predBlock |
       hasPhiNode(vvar, phiBlock) and
-      predBlock = phiBlock.getAPredecessor() and
+      predBlock = phiBlock.getAFeasiblePredecessor() and
       instr.getTag() = PhiTag(vvar, phiBlock) and
       newPredecessorBlock = getNewBlock(predBlock) and
       hasDefinitionAtRank(vvar, defBlock, defRank, defIndex) and
@@ -266,13 +262,20 @@ cached private module Cached {
       result = getChiInstruction(getOldInstruction(instruction)) and
       kind instanceof GotoEdge
     else (
-      result = getNewInstruction(getOldInstruction(instruction).getSuccessor(kind))
-      or
+      exists(OldInstruction oldInstruction |
+        oldInstruction = getOldInstruction(instruction) and
+        (
+          result.getTag() = UnreachedTag(oldInstruction, kind) or
+          (
+            result = getNewInstruction(oldInstruction.getSuccessor(kind)) and
+            not exists(UnreachedTag(oldInstruction, kind))
+          )
+        )
+      ) or
       exists(OldInstruction oldInstruction |
         instruction = getChiInstruction(oldInstruction) and
         result = getNewInstruction(oldInstruction.getSuccessor(kind))
-      ) or
-      result.getTag() = UnreachedTag(getOldInstruction(instruction), kind)
+      )
     )
   }
 
@@ -380,7 +383,7 @@ cached private module Cached {
 
   pragma[noinline]
   private predicate variableLiveOnExitFromBlock(Alias::VirtualVariable vvar, OldBlock block) {
-    variableLiveOnEntryToBlock(vvar, block.getASuccessor())
+    variableLiveOnEntryToBlock(vvar, block.getAFeasibleSuccessor())
   }
 
   /**
@@ -474,7 +477,7 @@ cached private module Cached {
         useRank) or
       (
         definitionReachesEndOfBlock(vvar, defBlock, defRank,
-          useBlock.getAPredecessor()) and
+          useBlock.getAFeasiblePredecessor()) and
         not definitionReachesUseWithinBlock(vvar, useBlock, _, useBlock, useRank)
       )
     )

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/reachability/PrintDominance.qll

@@ -0,0 +1,21 @@
+private import DominanceInternal
+private import ReachableBlockInternal
+private import Dominance
+import IR
+
+private class DominancePropertyProvider extends IRPropertyProvider {
+  override string getBlockProperty(IRBlock block, string key) {
+    exists(IRBlock dominator |
+      blockImmediatelyDominates(dominator, block) and
+      key = "ImmediateDominator" and
+      result = "Block " + dominator.getDisplayIndex().toString()
+    ) or
+    (
+      key = "DominanceFrontier" and
+      result = strictconcat(IRBlock frontierBlock |
+        frontierBlock = getDominanceFrontier(block) |
+        frontierBlock.getDisplayIndex().toString(), ", " order by frontierBlock.getDisplayIndex()
+      )
+    )
+  }
+}

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/reachability/ReachableBlock.qll

@@ -3,10 +3,9 @@ private import semmle.code.cpp.ir.internal.IntegerConstant
 private import IR
 private import ConstantAnalysis
 
-predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
-  exists(ConditionalBranchInstruction instr, int conditionValue |
-    instr = block.getLastInstruction() and
-    conditionValue = getValue(getConstantValue(instr.getCondition())) and
+predicate isInfeasibleInstructionSuccessor(Instruction instr, EdgeKind kind) {
+  exists(int conditionValue |
+    conditionValue = getValue(getConstantValue(instr.(ConditionalBranchInstruction).getCondition())) and
     if conditionValue = 0 then
       kind instanceof TrueEdge
     else
@@ -14,7 +13,11 @@ predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
   )
 }
 
-IRBlock getAFeasiblePredecessor(IRBlock successor) {
+predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
+  isInfeasibleInstructionSuccessor(block.getLastInstruction(), kind)
+}
+
+IRBlock getAFeasiblePredecessorBlock(IRBlock successor) {
   exists(EdgeKind kind |
     result.getSuccessor(kind) = successor and
     not isInfeasibleEdge(result, kind)
@@ -23,7 +26,7 @@ IRBlock getAFeasiblePredecessor(IRBlock successor) {
 
 predicate isBlockReachable(IRBlock block) {
   exists(FunctionIR f |
-    getAFeasiblePredecessor*(block) = f.getEntryBlock()
+    getAFeasiblePredecessorBlock*(block) = f.getEntryBlock()
   )
 }
 
@@ -35,6 +38,14 @@ class ReachableBlock extends IRBlock {
   ReachableBlock() {
     isBlockReachable(this)
   }
+
+  final ReachableBlock getAFeasiblePredecessor() {
+    result = getAFeasiblePredecessorBlock(this)
+  }
+
+  final ReachableBlock getAFeasibleSuccessor() {
+    this = getAFeasiblePredecessorBlock(result)
+  }
 }
 
 class ReachableInstruction extends Instruction {
@@ -51,6 +62,6 @@ module Graph {
   }
 
   predicate blockSuccessor(ReachableBlock pred, ReachableBlock succ) {
-    succ = pred.getASuccessor()
+    succ = pred.getAFeasibleSuccessor()
   }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll

@@ -26,13 +26,9 @@ cached private module Cached {
       hasChiNode(_, oldInstruction)
     } or
     UnreachedTag(OldInstruction oldInstruction, EdgeKind kind) {
-      // We need an `Unreached` instruction for the destination of any edge whose predecessor
-      // instruction is reachable, but whose successor block is not. This should occur only for
-      // infeasible edges.
-      exists(OldIR::Instruction succInstruction |
-        succInstruction = oldInstruction.getSuccessor(kind) and
-        not succInstruction instanceof OldInstruction
-      )
+      // We need an `Unreached` instruction for the destination of each infeasible edge whose
+      // predecessor is reachable.
+      Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind)
     }
 
   cached class InstructionTagType extends TInstructionTag {
@@ -213,7 +209,7 @@ cached private module Cached {
     exists(Alias::VirtualVariable vvar, OldBlock phiBlock,
         OldBlock defBlock, int defRank, int defIndex, OldBlock predBlock |
       hasPhiNode(vvar, phiBlock) and
-      predBlock = phiBlock.getAPredecessor() and
+      predBlock = phiBlock.getAFeasiblePredecessor() and
       instr.getTag() = PhiTag(vvar, phiBlock) and
       newPredecessorBlock = getNewBlock(predBlock) and
       hasDefinitionAtRank(vvar, defBlock, defRank, defIndex) and
@@ -266,13 +262,20 @@ cached private module Cached {
       result = getChiInstruction(getOldInstruction(instruction)) and
       kind instanceof GotoEdge
     else (
-      result = getNewInstruction(getOldInstruction(instruction).getSuccessor(kind))
-      or
+      exists(OldInstruction oldInstruction |
+        oldInstruction = getOldInstruction(instruction) and
+        (
+          result.getTag() = UnreachedTag(oldInstruction, kind) or
+          (
+            result = getNewInstruction(oldInstruction.getSuccessor(kind)) and
+            not exists(UnreachedTag(oldInstruction, kind))
+          )
+        )
+      ) or
       exists(OldInstruction oldInstruction |
         instruction = getChiInstruction(oldInstruction) and
         result = getNewInstruction(oldInstruction.getSuccessor(kind))
-      ) or
-      result.getTag() = UnreachedTag(getOldInstruction(instruction), kind)
+      )
     )
   }
 
@@ -380,7 +383,7 @@ cached private module Cached {
 
   pragma[noinline]
   private predicate variableLiveOnExitFromBlock(Alias::VirtualVariable vvar, OldBlock block) {
-    variableLiveOnEntryToBlock(vvar, block.getASuccessor())
+    variableLiveOnEntryToBlock(vvar, block.getAFeasibleSuccessor())
   }
 
   /**
@@ -474,7 +477,7 @@ cached private module Cached {
         useRank) or
       (
         definitionReachesEndOfBlock(vvar, defBlock, defRank,
-          useBlock.getAPredecessor()) and
+          useBlock.getAFeasiblePredecessor()) and
         not definitionReachesUseWithinBlock(vvar, useBlock, _, useBlock, useRank)
       )
     )

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/reachability/PrintDominance.qll

@@ -0,0 +1,21 @@
+private import DominanceInternal
+private import ReachableBlockInternal
+private import Dominance
+import IR
+
+private class DominancePropertyProvider extends IRPropertyProvider {
+  override string getBlockProperty(IRBlock block, string key) {
+    exists(IRBlock dominator |
+      blockImmediatelyDominates(dominator, block) and
+      key = "ImmediateDominator" and
+      result = "Block " + dominator.getDisplayIndex().toString()
+    ) or
+    (
+      key = "DominanceFrontier" and
+      result = strictconcat(IRBlock frontierBlock |
+        frontierBlock = getDominanceFrontier(block) |
+        frontierBlock.getDisplayIndex().toString(), ", " order by frontierBlock.getDisplayIndex()
+      )
+    )
+  }
+}

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/reachability/ReachableBlock.qll

@@ -3,10 +3,9 @@ private import semmle.code.cpp.ir.internal.IntegerConstant
 private import IR
 private import ConstantAnalysis
 
-predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
-  exists(ConditionalBranchInstruction instr, int conditionValue |
-    instr = block.getLastInstruction() and
-    conditionValue = getValue(getConstantValue(instr.getCondition())) and
+predicate isInfeasibleInstructionSuccessor(Instruction instr, EdgeKind kind) {
+  exists(int conditionValue |
+    conditionValue = getValue(getConstantValue(instr.(ConditionalBranchInstruction).getCondition())) and
     if conditionValue = 0 then
       kind instanceof TrueEdge
     else
@@ -14,7 +13,11 @@ predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
   )
 }
 
-IRBlock getAFeasiblePredecessor(IRBlock successor) {
+predicate isInfeasibleEdge(IRBlock block, EdgeKind kind) {
+  isInfeasibleInstructionSuccessor(block.getLastInstruction(), kind)
+}
+
+IRBlock getAFeasiblePredecessorBlock(IRBlock successor) {
   exists(EdgeKind kind |
     result.getSuccessor(kind) = successor and
     not isInfeasibleEdge(result, kind)
@@ -23,7 +26,7 @@ IRBlock getAFeasiblePredecessor(IRBlock successor) {
 
 predicate isBlockReachable(IRBlock block) {
   exists(FunctionIR f |
-    getAFeasiblePredecessor*(block) = f.getEntryBlock()
+    getAFeasiblePredecessorBlock*(block) = f.getEntryBlock()
   )
 }
 
@@ -35,6 +38,14 @@ class ReachableBlock extends IRBlock {
   ReachableBlock() {
     isBlockReachable(this)
   }
+
+  final ReachableBlock getAFeasiblePredecessor() {
+    result = getAFeasiblePredecessorBlock(this)
+  }
+
+  final ReachableBlock getAFeasibleSuccessor() {
+    this = getAFeasiblePredecessorBlock(result)
+  }
 }
 
 class ReachableInstruction extends Instruction {
@@ -51,6 +62,6 @@ module Graph {
   }
 
   predicate blockSuccessor(ReachableBlock pred, ReachableBlock succ) {
-    succ = pred.getASuccessor()
+    succ = pred.getAFeasibleSuccessor()
   }
 }

cpp/ql/test/library-tests/ir/constant_func/constant_func.cpp

@@ -58,3 +58,13 @@ int UnreachableIf(bool b) {
     }
   }
 }
+
+int DoWhileFalse() {
+  int i = 0;
+  do {
+    i++;
+  } while (false);
+
+  return i;
+}
+

cpp/ql/test/library-tests/ir/constant_func/constant_func.expected

@@ -3,3 +3,4 @@
 | constant_func.cpp:25:5:25:25 | IR: ReturnConstantPhiLoop | 7 |
 | constant_func.cpp:34:5:34:22 | IR: UnreachableViaGoto | 0 |
 | constant_func.cpp:41:5:41:17 | IR: UnreachableIf | 0 |
+| constant_func.cpp:62:5:62:16 | IR: DoWhileFalse | 1 |

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

@@ -6754,3 +6754,31 @@ ir.cpp:
 # 1044|                   Type = int
 # 1044|                   Value = 1
 # 1044|                   ValueCategory = prvalue
+# 1049| DoWhileFalse() -> int
+# 1049|   params: 
+# 1049|   body: { ... }
+# 1050|     0: declaration
+# 1050|       0: definition of i
+# 1050|           Type = int
+# 1050|         init: initializer for i
+# 1050|           expr: 0
+# 1050|               Type = int
+# 1050|               Value = 0
+# 1050|               ValueCategory = prvalue
+# 1051|     1: do (...) ...
+# 1053|       0: 0
+# 1053|           Type = bool
+# 1053|           Value = 0
+# 1053|           ValueCategory = prvalue
+# 1051|       1: { ... }
+# 1052|         0: ExprStmt
+# 1052|           0: ... ++
+# 1052|               Type = int
+# 1052|               ValueCategory = prvalue
+# 1052|             0: i
+# 1052|                 Type = int
+# 1052|                 ValueCategory = lvalue
+# 1055|     2: return ...
+# 1055|       0: i
+# 1055|           Type = int
+# 1055|           ValueCategory = prvalue(load)

cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected

@@ -4621,3 +4621,34 @@ ir.cpp:
 
 # 1044|   Block 7
 # 1044|     v7_0(void) = Unreached : 
+
+# 1049| DoWhileFalse() -> int
+# 1049|   Block 0
+# 1049|     v0_0(void)       = EnterFunction       : 
+# 1049|     m0_1(unknown)    = AliasedDefinition   : 
+# 1049|     mu0_2(unknown)   = UnmodeledDefinition : 
+# 1050|     r0_3(glval<int>) = VariableAddress[i]  : 
+# 1050|     r0_4(int)        = Constant[0]         : 
+# 1050|     m0_5(int)        = Store               : r0_3, r0_4
+# 1052|     r0_6(glval<int>) = VariableAddress[i]  : 
+# 1052|     r0_7(int)        = Load                : r0_6, m0_5
+# 1052|     r0_8(int)        = Constant[1]         : 
+# 1052|     r0_9(int)        = Add                 : r0_7, r0_8
+# 1052|     m0_10(int)       = Store               : r0_6, r0_9
+# 1053|     r0_11(bool)      = Constant[0]         : 
+# 1053|     v0_12(void)      = ConditionalBranch   : r0_11
+#-----|   False -> Block 1
+#-----|   True -> Block 2
+
+# 1055|   Block 1
+# 1055|     r1_0(glval<int>) = VariableAddress[#return] : 
+# 1055|     r1_1(glval<int>) = VariableAddress[i]       : 
+# 1055|     r1_2(int)        = Load                     : r1_1, m0_10
+# 1055|     m1_3(int)        = Store                    : r1_0, r1_2
+# 1049|     r1_4(glval<int>) = VariableAddress[#return] : 
+# 1049|     v1_5(void)       = ReturnValue              : r1_4, m1_3
+# 1049|     v1_6(void)       = UnmodeledUse             : mu*
+# 1049|     v1_7(void)       = ExitFunction             : 
+
+# 1052|   Block 2
+# 1052|     v2_0(void) = Unreached : 

cpp/ql/test/library-tests/ir/ir/ir.cpp

@@ -1046,4 +1046,13 @@ int UnreachableIf(bool b) {
   }
 }
 
+int DoWhileFalse() {
+  int i = 0;
+  do {
+    i++;
+  } while (false);
+
+  return i;
+}
+
 // semmle-extractor-options: -std=c++17

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

@@ -4513,3 +4513,34 @@ ir.cpp:
 # 1044|     r7_1(int)        = Constant[1]              : 
 # 1044|     mu7_2(int)       = Store                    : r7_0, r7_1
 #-----|   Goto -> Block 1
+
+# 1049| DoWhileFalse() -> int
+# 1049|   Block 0
+# 1049|     v0_0(void)       = EnterFunction       : 
+# 1049|     mu0_1(unknown)   = AliasedDefinition   : 
+# 1049|     mu0_2(unknown)   = UnmodeledDefinition : 
+# 1050|     r0_3(glval<int>) = VariableAddress[i]  : 
+# 1050|     r0_4(int)        = Constant[0]         : 
+# 1050|     mu0_5(int)       = Store               : r0_3, r0_4
+#-----|   Goto -> Block 1
+
+# 1052|   Block 1
+# 1052|     r1_0(glval<int>) = VariableAddress[i] : 
+# 1052|     r1_1(int)        = Load               : r1_0, mu0_2
+# 1052|     r1_2(int)        = Constant[1]        : 
+# 1052|     r1_3(int)        = Add                : r1_1, r1_2
+# 1052|     mu1_4(int)       = Store              : r1_0, r1_3
+# 1053|     r1_5(bool)       = Constant[0]        : 
+# 1053|     v1_6(void)       = ConditionalBranch  : r1_5
+#-----|   False -> Block 2
+#-----|   True -> Block 1
+
+# 1055|   Block 2
+# 1055|     r2_0(glval<int>) = VariableAddress[#return] : 
+# 1055|     r2_1(glval<int>) = VariableAddress[i]       : 
+# 1055|     r2_2(int)        = Load                     : r2_1, mu0_2
+# 1055|     mu2_3(int)       = Store                    : r2_0, r2_2
+# 1049|     r2_4(glval<int>) = VariableAddress[#return] : 
+# 1049|     v2_5(void)       = ReturnValue              : r2_4, mu0_2
+# 1049|     v2_6(void)       = UnmodeledUse             : mu*
+# 1049|     v2_7(void)       = ExitFunction             : 

cpp/ql/test/library-tests/ir/ir/ssa_block_count.expected

@@ -31,6 +31,7 @@
 | IR: Derived | 1 |
 | IR: DerivedVB | 1 |
 | IR: DoStatements | 3 |
+| IR: DoWhileFalse | 3 |
 | IR: DynamicCast | 1 |
 | IR: EarlyReturn | 4 |
 | IR: EarlyReturnValue | 4 |

cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected

@@ -4486,3 +4486,34 @@ ir.cpp:
 # 1044|     r7_1(int)        = Constant[1]              : 
 # 1044|     m7_2(int)        = Store                    : r7_0, r7_1
 #-----|   Goto -> Block 1
+
+# 1049| DoWhileFalse() -> int
+# 1049|   Block 0
+# 1049|     v0_0(void)       = EnterFunction       : 
+# 1049|     mu0_1(unknown)   = AliasedDefinition   : 
+# 1049|     mu0_2(unknown)   = UnmodeledDefinition : 
+# 1050|     r0_3(glval<int>) = VariableAddress[i]  : 
+# 1050|     r0_4(int)        = Constant[0]         : 
+# 1050|     m0_5(int)        = Store               : r0_3, r0_4
+# 1052|     r0_6(glval<int>) = VariableAddress[i]  : 
+# 1052|     r0_7(int)        = Load                : r0_6, m0_5
+# 1052|     r0_8(int)        = Constant[1]         : 
+# 1052|     r0_9(int)        = Add                 : r0_7, r0_8
+# 1052|     m0_10(int)       = Store               : r0_6, r0_9
+# 1053|     r0_11(bool)      = Constant[0]         : 
+# 1053|     v0_12(void)      = ConditionalBranch   : r0_11
+#-----|   False -> Block 1
+#-----|   True -> Block 2
+
+# 1055|   Block 1
+# 1055|     r1_0(glval<int>) = VariableAddress[#return] : 
+# 1055|     r1_1(glval<int>) = VariableAddress[i]       : 
+# 1055|     r1_2(int)        = Load                     : r1_1, m0_10
+# 1055|     m1_3(int)        = Store                    : r1_0, r1_2
+# 1049|     r1_4(glval<int>) = VariableAddress[#return] : 
+# 1049|     v1_5(void)       = ReturnValue              : r1_4, m1_3
+# 1049|     v1_6(void)       = UnmodeledUse             : mu*
+# 1049|     v1_7(void)       = ExitFunction             : 
+
+# 1052|   Block 2
+# 1052|     v2_0(void) = Unreached :