Merge pull request #694 from dave-bartolomeo/dave/BetterUnreached

C++: Remove infeasible edges to reachable blocks

Author: jbj

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/controlflow/IRGuards.qll

@@ -1,6 +1,17 @@
 import cpp
 import semmle.code.cpp.ir.IR
 
+/**
+ * Holds if `block` consists of an `UnreachedInstruction`.
+ *
+ * We avoiding reporting an unreached block as being controlled by a guard. The unreached block
+ * has the AST for the `Function` itself, which tends to confuse mapping between the AST `BasicBlock`
+ * and the `IRBlock`.
+ */
+private predicate isUnreachedBlock(IRBlock block) {
+  block.getFirstInstruction() instanceof UnreachedInstruction
+}
+
 /**
  * A Boolean condition in the AST that guards one or more basic blocks. This includes
  * operands of logical operators but not switch statements.
@@ -215,7 +226,8 @@ private class GuardConditionFromIR extends GuardCondition {
     private predicate controlsBlock(BasicBlock controlled, boolean testIsTrue) {
       exists(IRBlock irb |
         forex(IRGuardCondition inst | inst = ir | inst.controls(irb, testIsTrue)) and
-        irb.getAnInstruction().getAST().(ControlFlowNode).getBasicBlock() = controlled
+        irb.getAnInstruction().getAST().(ControlFlowNode).getBasicBlock() = controlled and
+        not isUnreachedBlock(irb)
       )
     }
 }
@@ -301,6 +313,7 @@ class IRGuardCondition extends Instruction {
      * `&&` and `||`. See the detailed explanation on predicate `controls`.
      */
     private predicate controlsBlock(IRBlock controlled, boolean testIsTrue) {
+        not isUnreachedBlock(controlled) and
         exists(IRBlock thisblock
         | thisblock.getAnInstruction() = this
         | exists(IRBlock succ, ConditionalBranchInstruction branch

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll

@@ -3,7 +3,18 @@ import Instruction
 import semmle.code.cpp.ir.implementation.EdgeKind
 private import Cached
 
-class IRBlock extends TIRBlock {
+/**
+ * A basic block in the IR. A basic block consists of a sequence of `Instructions` with the only
+ * incoming edges at the beginning of the sequence and the only outgoing edges at the end of the
+ * sequence.
+ *
+ * This class does not contain any members that query the predecessor or successor edges of the
+ * block. This allows different classes that extend `IRBlockBase` to expose different subsets of
+ * edges (e.g. ignoring unreachable edges).
+ *
+ * Most consumers should use the class `IRBlock`.
+ */
+class IRBlockBase extends TIRBlock {
   final string toString() {
     result = getFirstInstruction(this).toString()
   }
@@ -59,7 +70,14 @@ class IRBlock extends TIRBlock {
   final Function getFunction() {
     result = getFirstInstruction(this).getFunction()
   }
+}
 
+/**
+ * A basic block with additional information about its predecessor and successor edges. Each edge
+ * corresponds to the control flow between the last instruction of one block and the first
+ * instruction of another block.
+ */
+class IRBlock extends IRBlockBase {
   final IRBlock getASuccessor() {
     blockSuccessor(this, result)
   }

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

@@ -25,15 +25,7 @@ cached private module Cached {
       not oldInstruction instanceof OldIR::PhiInstruction and
       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
-      )
-    }
+    UnreachedTag()
 
   cached class InstructionTagType extends TInstructionTag {
     cached final string toString() {
@@ -133,11 +125,12 @@ cached private module Cached {
       resultType = vvar.getType() and
       isGLValue = false
     ) or
-    exists(OldInstruction oldInstruction, EdgeKind kind |
-      oldInstruction.getFunction() = func and
-      tag = UnreachedTag(oldInstruction, kind) and
+    exists(OldInstruction oldInstruction |
+      func = oldInstruction.getFunction() and
+      Reachability::isInfeasibleInstructionSuccessor(oldInstruction, _) and
+      tag = UnreachedTag() and
       opcode instanceof Opcode::Unreached and
-      ast = oldInstruction.getSuccessor(kind).getAST() and
+      ast = func and
       resultType instanceof VoidType and
       isGLValue = false
     )
@@ -213,7 +206,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 +259,22 @@ 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
+        (
+          if Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) then (
+            result.getTag() = UnreachedTag() and
+            result.getFunction() = instruction.getFunction()
+          )
+          else (
+            result = getNewInstruction(oldInstruction.getSuccessor(kind))
+          )
+        )
+      ) or
       exists(OldInstruction oldInstruction |
         instruction = getChiInstruction(oldInstruction) and
         result = getNewInstruction(oldInstruction.getSuccessor(kind))
-      ) or
-      result.getTag() = UnreachedTag(getOldInstruction(instruction), kind)
+      )
     )
   }
 
@@ -380,7 +382,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 +476,7 @@ cached private module Cached {
         useRank) or
       (
         definitionReachesEndOfBlock(vvar, defBlock, defRank,
-          useBlock.getAPredecessor()) and
+          useBlock.getAFeasiblePredecessor()) and
         not definitionReachesUseWithinBlock(vvar, useBlock, _, useBlock, useRank)
       )
     )
@@ -518,9 +520,9 @@ cached private module CachedForDebugging {
       instr.getTag() = PhiTag(vvar, phiBlock) and
       result = "Phi Block(" + phiBlock.getUniqueId() + "): " + vvar.getUniqueId() 
     ) or
-    exists(OldInstruction oldInstr, EdgeKind kind |
-      instr.getTag() = UnreachedTag(oldInstr, kind) and
-      result = "Unreached(" + oldInstr.getUniqueId() + ":" + kind.toString() + ")"
+    (
+      instr.getTag() = UnreachedTag() and
+      result = "Unreached"
     )
   }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll

@@ -3,7 +3,18 @@ import Instruction
 import semmle.code.cpp.ir.implementation.EdgeKind
 private import Cached
 
-class IRBlock extends TIRBlock {
+/**
+ * A basic block in the IR. A basic block consists of a sequence of `Instructions` with the only
+ * incoming edges at the beginning of the sequence and the only outgoing edges at the end of the
+ * sequence.
+ *
+ * This class does not contain any members that query the predecessor or successor edges of the
+ * block. This allows different classes that extend `IRBlockBase` to expose different subsets of
+ * edges (e.g. ignoring unreachable edges).
+ *
+ * Most consumers should use the class `IRBlock`.
+ */
+class IRBlockBase extends TIRBlock {
   final string toString() {
     result = getFirstInstruction(this).toString()
   }
@@ -59,7 +70,14 @@ class IRBlock extends TIRBlock {
   final Function getFunction() {
     result = getFirstInstruction(this).getFunction()
   }
+}
 
+/**
+ * A basic block with additional information about its predecessor and successor edges. Each edge
+ * corresponds to the control flow between the last instruction of one block and the first
+ * instruction of another block.
+ */
+class IRBlock extends IRBlockBase {
   final IRBlock getASuccessor() {
     blockSuccessor(this, result)
   }

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,40 +3,54 @@ 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
       kind instanceof FalseEdge
   )
 }
 
-IRBlock getAFeasiblePredecessor(IRBlock successor) {
+predicate isInfeasibleEdge(IRBlockBase block, EdgeKind kind) {
+  isInfeasibleInstructionSuccessor(block.getLastInstruction(), kind)
+}
+
+private IRBlock getAFeasiblePredecessorBlock(IRBlock successor) {
   exists(EdgeKind kind |
     result.getSuccessor(kind) = successor and
     not isInfeasibleEdge(result, kind)
   )
 }
 
-predicate isBlockReachable(IRBlock block) {
+private predicate isBlockReachable(IRBlock block) {
   exists(FunctionIR f |
-    getAFeasiblePredecessor*(block) = f.getEntryBlock()
+    getAFeasiblePredecessorBlock*(block) = f.getEntryBlock()
   )
 }
 
-predicate isInstructionReachable(Instruction instr) {
-  isBlockReachable(instr.getBlock())
-}
-
-class ReachableBlock extends IRBlock {
+/**
+ * An IR block that is reachable from the entry block of the function, considering only feasible
+ * edges.
+ */
+class ReachableBlock extends IRBlockBase {
   ReachableBlock() {
     isBlockReachable(this)
   }
+
+  final ReachableBlock getAFeasiblePredecessor() {
+    result = getAFeasiblePredecessorBlock(this)
+  }
+
+  final ReachableBlock getAFeasibleSuccessor() {
+    this = getAFeasiblePredecessorBlock(result)
+  }
 }
 
+/**
+ * An instruction that is contained in a reachable block.
+ */
 class ReachableInstruction extends Instruction {
   ReachableInstruction() {
     this.getBlock() instanceof ReachableBlock
@@ -51,6 +65,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/IRBlock.qll

@@ -3,7 +3,18 @@ import Instruction
 import semmle.code.cpp.ir.implementation.EdgeKind
 private import Cached
 
-class IRBlock extends TIRBlock {
+/**
+ * A basic block in the IR. A basic block consists of a sequence of `Instructions` with the only
+ * incoming edges at the beginning of the sequence and the only outgoing edges at the end of the
+ * sequence.
+ *
+ * This class does not contain any members that query the predecessor or successor edges of the
+ * block. This allows different classes that extend `IRBlockBase` to expose different subsets of
+ * edges (e.g. ignoring unreachable edges).
+ *
+ * Most consumers should use the class `IRBlock`.
+ */
+class IRBlockBase extends TIRBlock {
   final string toString() {
     result = getFirstInstruction(this).toString()
   }
@@ -59,7 +70,14 @@ class IRBlock extends TIRBlock {
   final Function getFunction() {
     result = getFirstInstruction(this).getFunction()
   }
+}
 
+/**
+ * A basic block with additional information about its predecessor and successor edges. Each edge
+ * corresponds to the control flow between the last instruction of one block and the first
+ * instruction of another block.
+ */
+class IRBlock extends IRBlockBase {
   final IRBlock getASuccessor() {
     blockSuccessor(this, result)
   }