Merge pull request github#12356 from MathiasVP/use-phi-reads

C++: Include "phi reads" in `DataFlow::Node`

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -43,7 +43,7 @@ class Node0Impl extends TIRDataFlowNode0 {
   /**
    * Gets the type of this node.
    *
-   * If `asInstruction().isGLValue()` holds, then the type of this node
+   * If `isGLValue()` holds, then the type of this node
    * should be thought of as "pointer to `getType()`".
    */
   DataFlowType getType() { none() } // overridden in subclasses

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -498,7 +498,14 @@ class SsaPhiNode extends Node, TSsaPhiNode {
 
   override Declaration getFunction() { result = phi.getBasicBlock().getEnclosingFunction() }
 
-  override DataFlowType getType() { result = this.getAnInput().getType().getUnspecifiedType() }
+  override DataFlowType getType() {
+    exists(Ssa::SourceVariable sv |
+      this.getPhiNode().definesAt(sv, _, _, _) and
+      result = sv.getType()
+    )
+  }
+
+  override predicate isGLValue() { phi.getSourceVariable().isGLValue() }
 
   final override Location getLocationImpl() { result = phi.getBasicBlock().getLocation() }
 

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -48,6 +48,16 @@ private module SourceVariables {
      * indirections) of this source variable.
      */
     abstract BaseSourceVariable getBaseVariable();
+
+    /** Holds if this variable is a glvalue. */
+    predicate isGLValue() { none() }
+
+    /**
+     * Gets the type of this source variable. If `isGLValue()` holds, then
+     * the type of this source variable should be thought of as "pointer
+     * to `getType()`".
+     */
+    abstract DataFlowType getType();
   }
 
   class SourceIRVariable extends SourceVariable, TSourceIRVariable {
@@ -66,6 +76,12 @@ private module SourceVariables {
       ind > 0 and
       result = this.getIRVariable().toString() + " indirection"
     }
+
+    override predicate isGLValue() { ind = 0 }
+
+    override DataFlowType getType() {
+      if ind = 0 then result = var.getType() else result = getTypeImpl(var.getType(), ind - 1)
+    }
   }
 
   class CallVariable extends SourceVariable, TCallVariable {
@@ -84,6 +100,8 @@ private module SourceVariables {
       ind > 0 and
       result = "Call indirection"
     }
+
+    override DataFlowType getType() { result = getTypeImpl(call.getResultType(), ind) }
   }
 }
 
@@ -530,15 +548,15 @@ predicate adjacentDefRead(DefOrUse defOrUse1, UseOrPhi use) {
   exists(IRBlock bb1, int i1, SourceVariable v |
     defOrUse1.asDefOrUse().hasIndexInBlock(bb1, i1, v)
   |
-    exists(IRBlock bb2, int i2, Definition def |
-      adjacentDefRead(pragma[only_bind_into](def), pragma[only_bind_into](bb1),
+    exists(IRBlock bb2, int i2, DefinitionExt def |
+      adjacentDefReadExt(pragma[only_bind_into](def), pragma[only_bind_into](bb1),
         pragma[only_bind_into](i1), pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and
       def.getSourceVariable() = v and
       use.asDefOrUse().(UseImpl).hasIndexInBlock(bb2, i2, v)
     )
     or
     exists(PhiNode phi |
-      lastRefRedef(_, bb1, i1, phi) and
+      lastRefRedefExt(_, bb1, i1, phi) and
       use.asPhi() = phi and
       phi.getSourceVariable() = pragma[only_bind_into](v)
     )
@@ -550,11 +568,18 @@ predicate adjacentDefRead(DefOrUse defOrUse1, UseOrPhi use) {
  * flows to `useOrPhi`.
  */
 private predicate globalDefToUse(GlobalDef globalDef, UseOrPhi useOrPhi) {
-  exists(IRBlock bb1, int i1, IRBlock bb2, int i2, SourceVariable v |
-    globalDef.hasIndexInBlock(bb1, i1, v) and
-    adjacentDefRead(_, pragma[only_bind_into](bb1), pragma[only_bind_into](i1),
-      pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and
-    useOrPhi.asDefOrUse().hasIndexInBlock(bb2, i2, v)
+  exists(IRBlock bb1, int i1, SourceVariable v | globalDef.hasIndexInBlock(bb1, i1, v) |
+    exists(IRBlock bb2, int i2 |
+      adjacentDefReadExt(_, pragma[only_bind_into](bb1), pragma[only_bind_into](i1),
+        pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and
+      useOrPhi.asDefOrUse().hasIndexInBlock(bb2, i2, v)
+    )
+    or
+    exists(PhiNode phi |
+      lastRefRedefExt(_, bb1, i1, phi) and
+      useOrPhi.asPhi() = phi and
+      phi.getSourceVariable() = pragma[only_bind_into](v)
+    )
   )
 }
 
@@ -666,17 +691,17 @@ private predicate ssaFlowImpl(SsaDefOrUse defOrUse, Node nodeFrom, Node nodeTo,
  * Holds if `def` is the corresponding definition of
  * the SSA library's `definition`.
  */
-private Definition ssaDefinition(Def def) {
+private DefinitionExt ssaDefinition(Def def) {
   exists(IRBlock block, int i, SourceVariable sv |
     def.hasIndexInBlock(block, i, sv) and
-    result.definesAt(sv, block, i)
+    result.definesAt(sv, block, i, _)
   )
 }
 
 /** Gets a node that represents the prior definition of `node`. */
 private Node getAPriorDefinition(SsaDefOrUse defOrUse) {
-  exists(IRBlock bb, int i, SourceVariable sv, Definition def, DefOrUse defOrUse0 |
-    SsaCached::lastRefRedef(pragma[only_bind_into](def), pragma[only_bind_into](bb),
+  exists(IRBlock bb, int i, SourceVariable sv, DefinitionExt def, DefOrUse defOrUse0 |
+    lastRefRedefExt(pragma[only_bind_into](def), pragma[only_bind_into](bb),
       pragma[only_bind_into](i), ssaDefinition(defOrUse)) and
     def.getSourceVariable() = sv and
     defOrUse0.hasIndexInBlock(bb, i, sv) and
@@ -702,7 +727,7 @@ pragma[nomagic]
 private predicate fromPhiNodeToUse(PhiNode phi, SourceVariable sv, IRBlock bb1, int i1, UseOrPhi use) {
   exists(IRBlock bb2, int i2 |
     use.asDefOrUse().hasIndexInBlock(bb2, i2, sv) and
-    adjacentDefRead(pragma[only_bind_into](phi), pragma[only_bind_into](bb1),
+    adjacentDefReadExt(pragma[only_bind_into](phi), pragma[only_bind_into](bb1),
       pragma[only_bind_into](i1), pragma[only_bind_into](bb2), pragma[only_bind_into](i2))
   )
 }
@@ -711,13 +736,13 @@ private predicate fromPhiNodeToUse(PhiNode phi, SourceVariable sv, IRBlock bb1,
 predicate fromPhiNode(SsaPhiNode nodeFrom, Node nodeTo) {
   exists(PhiNode phi, SourceVariable sv, IRBlock bb1, int i1, UseOrPhi use |
     phi = nodeFrom.getPhiNode() and
-    phi.definesAt(sv, bb1, i1) and
+    phi.definesAt(sv, bb1, i1, _) and
     useToNode(use, nodeTo)
   |
     fromPhiNodeToUse(phi, sv, bb1, i1, use)
     or
     exists(PhiNode phiTo |
-      lastRefRedef(phi, _, _, phiTo) and
+      lastRefRedefExt(phi, _, _, phiTo) and
       nodeTo.(SsaPhiNode).getPhiNode() = phiTo
     )
   )
@@ -796,8 +821,8 @@ module SsaCached {
    * path between them without any read of `def`.
    */
   cached
-  predicate adjacentDefRead(Definition def, IRBlock bb1, int i1, IRBlock bb2, int i2) {
-    SsaImpl::adjacentDefRead(def, bb1, i1, bb2, i2)
+  predicate adjacentDefReadExt(DefinitionExt def, IRBlock bb1, int i1, IRBlock bb2, int i2) {
+    SsaImpl::adjacentDefReadExt(def, _, bb1, i1, bb2, i2)
   }
 
   /**
@@ -806,8 +831,8 @@ module SsaCached {
    * without passing through another read or write.
    */
   cached
-  predicate lastRefRedef(Definition def, IRBlock bb, int i, Definition next) {
-    SsaImpl::lastRefRedef(def, bb, i, next)
+  predicate lastRefRedefExt(DefinitionExt def, IRBlock bb, int i, DefinitionExt next) {
+    SsaImpl::lastRefRedefExt(def, _, bb, i, next)
   }
 }
 
@@ -818,8 +843,8 @@ private newtype TSsaDefOrUse =
     or
     // Like in the pruning stage, we only include definition that's live after the
     // write as the final definitions computed by SSA.
-    exists(Definition def, SourceVariable sv, IRBlock bb, int i |
-      def.definesAt(sv, bb, i) and
+    exists(DefinitionExt def, SourceVariable sv, IRBlock bb, int i |
+      def.definesAt(sv, bb, i, _) and
       defOrUse.(DefImpl).hasIndexInBlock(bb, i, sv)
     )
   } or
@@ -967,9 +992,14 @@ class Def extends DefOrUse {
 
 private module SsaImpl = SsaImplCommon::Make<SsaInput>;
 
-class PhiNode = SsaImpl::PhiNode;
+class PhiNode extends SsaImpl::DefinitionExt {
+  PhiNode() {
+    this instanceof SsaImpl::PhiNode or
+    this instanceof SsaImpl::PhiReadNode
+  }
+}
 
-class Definition = SsaImpl::Definition;
+class DefinitionExt = SsaImpl::DefinitionExt;
 
 class UncertainWriteDefinition = SsaImpl::UncertainWriteDefinition;
 

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll

@@ -225,8 +225,8 @@ private newtype TSsaDefOrUse =
     or
     // If `defOrUse` is a definition we only include it if the
     // SSA library concludes that it's live after the write.
-    exists(Definition def, SourceVariable sv, IRBlock bb, int i |
-      def.definesAt(sv, bb, i) and
+    exists(DefinitionExt def, SourceVariable sv, IRBlock bb, int i |
+      def.definesAt(sv, bb, i, _) and
       defOrUse.(DefImpl).hasIndexInBlock(bb, i, sv)
     )
   } or
@@ -301,6 +301,11 @@ class Def extends DefOrUse {
 
 private module SsaImpl = SsaImplCommon::Make<SsaInput>;
 
-class PhiNode = SsaImpl::PhiNode;
+class PhiNode extends SsaImpl::DefinitionExt {
+  PhiNode() {
+    this instanceof SsaImpl::PhiNode or
+    this instanceof SsaImpl::PhiReadNode
+  }
+}
 
-class Definition = SsaImpl::Definition;
+class DefinitionExt = SsaImpl::DefinitionExt;

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

@@ -615,3 +615,16 @@ void test_flow_through_void_double_pointer(int *p) // $ ast-def=p
   void* q = (void*)&p;
   sink(**(int**)q); // $ ir MISSING: ast
 }
+
+void use(int *);
+
+void test_def_via_phi_read(bool b)
+{
+  static int buffer[10]; // This is missing an initialisation in IR dataflow
+  if (b)
+  {
+    use(buffer);
+  }
+  intPointerSource(buffer);
+  sink(buffer); // $ ast,ir
+}