Merge pull request github#10713 from MathiasVP/fix-types-in-ir-dataflow

C++: Fix `getType` for experimental IR dataflow

Author: MathiasVP

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

@@ -137,7 +137,7 @@ private newtype TReturnKind =
     exists(IndirectReturnNode return, ReturnIndirectionInstruction returnInd |
       returnInd.hasIndex(argumentIndex) and
       return.getAddressOperand() = returnInd.getSourceAddressOperand() and
-      indirectionIndex = return.getIndirectionIndex() - 1 // We subtract one because the return loads the value.
+      indirectionIndex = return.getIndirectionIndex()
     )
   }
 
@@ -197,7 +197,7 @@ class ReturnIndirectionNode extends IndirectReturnNode, ReturnNode {
     exists(int argumentIndex, ReturnIndirectionInstruction returnInd |
       returnInd.hasIndex(argumentIndex) and
       this.getAddressOperand() = returnInd.getSourceAddressOperand() and
-      result = TIndirectReturnKind(argumentIndex, this.getIndirectionIndex() - 1) and
+      result = TIndirectReturnKind(argumentIndex, this.getIndirectionIndex()) and
       hasNonInitializeParameterDef(returnInd.getIRVariable())
     )
     or
@@ -365,7 +365,7 @@ predicate jumpStep(Node n1, Node n2) {
 predicate storeStep(Node node1, Content c, PostFieldUpdateNode node2) {
   exists(int indirectionIndex1, int numberOfLoads, StoreInstruction store |
     nodeHasInstruction(node1, store, pragma[only_bind_into](indirectionIndex1)) and
-    node2.getIndirectionIndex() = 0 and
+    node2.getIndirectionIndex() = 1 and
     numberOfLoadsFromOperand(node2.getFieldAddress(), store.getDestinationAddressOperand(),
       numberOfLoads)
   |
@@ -465,20 +465,20 @@ predicate clearsContent(Node n, Content c) {
 predicate expectsContent(Node n, ContentSet c) { none() }
 
 /** Gets the type of `n` used for type pruning. */
-IRType getNodeType(Node n) {
+DataFlowType getNodeType(Node n) {
   suppressUnusedNode(n) and
-  result instanceof IRVoidType // stub implementation
+  result instanceof VoidType // stub implementation
 }
 
 /** Gets a string representation of a type returned by `getNodeType`. */
-string ppReprType(IRType t) { none() } // stub implementation
+string ppReprType(DataFlowType t) { none() } // stub implementation
 
 /**
  * Holds if `t1` and `t2` are compatible, that is, whether data can flow from
  * a node of type `t1` to a node of type `t2`.
  */
 pragma[inline]
-predicate compatibleTypes(IRType t1, IRType t2) {
+predicate compatibleTypes(DataFlowType t1, DataFlowType t2) {
   any() // stub implementation
 }
 
@@ -502,7 +502,7 @@ class DataFlowCallable = Cpp::Declaration;
 
 class DataFlowExpr = Expr;
 
-class DataFlowType = IRType;
+class DataFlowType = Type;
 
 /** A function call relevant for data flow. */
 class DataFlowCall extends CallInstruction {

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

@@ -38,13 +38,12 @@ private module Cached {
     TVariableNode(Variable var) or
     TPostFieldUpdateNode(FieldAddress operand, int indirectionIndex) {
       indirectionIndex =
-        [0 .. Ssa::countIndirectionsForCppType(operand.getObjectAddress().getResultLanguageType()) -
-            1]
+        [1 .. Ssa::countIndirectionsForCppType(operand.getObjectAddress().getResultLanguageType())]
     } or
     TSsaPhiNode(Ssa::PhiNode phi) or
     TIndirectArgumentOutNode(ArgumentOperand operand, int indirectionIndex) {
       Ssa::isModifiableByCall(operand) and
-      indirectionIndex = [0 .. Ssa::countIndirectionsForCppType(operand.getLanguageType()) - 1]
+      indirectionIndex = [1 .. Ssa::countIndirectionsForCppType(operand.getLanguageType())]
     } or
     TIndirectOperand(Operand op, int indirectionIndex) {
       Ssa::hasIndirectOperand(op, indirectionIndex)
@@ -113,7 +112,7 @@ class Node extends TIRDataFlowNode {
   Declaration getFunction() { none() } // overridden in subclasses
 
   /** Gets the type of this node. */
-  IRType getType() { none() } // overridden in subclasses
+  DataFlowType getType() { none() } // overridden in subclasses
 
   /** Gets the instruction corresponding to this node, if any. */
   Instruction asInstruction() { result = this.(InstructionNode).getInstruction() }
@@ -273,7 +272,7 @@ class Node extends TIRDataFlowNode {
   /**
    * Gets an upper bound on the type of this node.
    */
-  IRType getTypeBound() { result = this.getType() }
+  DataFlowType getTypeBound() { result = this.getType() }
 
   /** Gets the location of this element. */
   cached
@@ -322,7 +321,7 @@ class InstructionNode extends Node, TInstructionNode {
 
   override Declaration getFunction() { result = instr.getEnclosingFunction() }
 
-  override IRType getType() { result = instr.getResultIRType() }
+  override DataFlowType getType() { result = instr.getResultType() }
 
   final override Location getLocationImpl() { result = instr.getLocation() }
 
@@ -348,13 +347,32 @@ class OperandNode extends Node, TOperandNode {
 
   override Declaration getFunction() { result = op.getUse().getEnclosingFunction() }
 
-  override IRType getType() { result = op.getIRType() }
+  override DataFlowType getType() { result = op.getType() }
 
   final override Location getLocationImpl() { result = op.getLocation() }
 
   override string toStringImpl() { result = this.getOperand().toString() }
 }
 
+/**
+ * Returns `t`, but stripped of the `n` outermost pointers, references, etc.
+ *
+ * For example, `stripPointers(int*&, 2)` is `int` and `stripPointers(int*, 0)` is `int*`.
+ */
+private Type stripPointers(Type t, int n) {
+  result = t and n = 0
+  or
+  result = stripPointers(t.(PointerType).getBaseType(), n - 1)
+  or
+  result = stripPointers(t.(ArrayType).getBaseType(), n - 1)
+  or
+  result = stripPointers(t.(ReferenceType).getBaseType(), n - 1)
+  or
+  result = stripPointers(t.(PointerToMemberType).getBaseType(), n - 1)
+  or
+  result = stripPointers(t.(FunctionPointerIshType).getBaseType(), n - 1)
+}
+
 /**
  * INTERNAL: do not use.
  *
@@ -370,19 +388,15 @@ class PostFieldUpdateNode extends TPostFieldUpdateNode, PartialDefinitionNode {
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }
 
-  override IRType getType() { result = fieldAddress.getIRType() }
-
   FieldAddress getFieldAddress() { result = fieldAddress }
 
   Field getUpdatedField() { result = fieldAddress.getField() }
 
   int getIndirectionIndex() { result = indirectionIndex }
 
   override Node getPreUpdateNode() {
-    // + 1 because we're storing into an lvalue, and the original node should be the rvalue of
-    // the same address.
     hasOperandAndIndex(result, pragma[only_bind_into](fieldAddress).getObjectAddressOperand(),
-      indirectionIndex + 1)
+      indirectionIndex)
   }
 
   override Expr getDefinedExpr() {
@@ -411,7 +425,7 @@ class SsaPhiNode extends Node, TSsaPhiNode {
 
   override Declaration getFunction() { result = phi.getBasicBlock().getEnclosingFunction() }
 
-  override IRType getType() { result instanceof IRVoidType }
+  override DataFlowType getType() { result = this.getAnInput().getType() }
 
   final override Location getLocationImpl() { result = phi.getBasicBlock().getLocation() }
 
@@ -454,8 +468,6 @@ class SideEffectOperandNode extends Node, IndirectOperand {
 
   override Function getFunction() { result = call.getEnclosingFunction() }
 
-  override IRType getType() { result instanceof IRVoidType }
-
   Expr getArgument() { result = call.getArgument(argumentIndex).getUnconvertedResultExpression() }
 }
 
@@ -478,8 +490,6 @@ class IndirectParameterNode extends Node, IndirectInstruction {
 
   override Function getFunction() { result = this.getInstruction().getEnclosingFunction() }
 
-  override IRType getType() { result instanceof IRVoidType }
-
   override string toStringImpl() {
     result = this.getParameter().toString() + " indirection"
     or
@@ -504,8 +514,6 @@ class IndirectReturnNode extends IndirectOperand {
   Operand getAddressOperand() { result = operand }
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }
-
-  override IRType getType() { result instanceof IRVoidType }
 }
 
 /**
@@ -536,9 +544,7 @@ class IndirectArgumentOutNode extends Node, TIndirectArgumentOutNode, PostUpdate
 
   override Function getFunction() { result = this.getCallInstruction().getEnclosingFunction() }
 
-  override IRType getType() { result instanceof IRVoidType }
-
-  override Node getPreUpdateNode() { hasOperandAndIndex(result, operand, indirectionIndex + 1) }
+  override Node getPreUpdateNode() { hasOperandAndIndex(result, operand, indirectionIndex) }
 
   override string toStringImpl() {
     // This string should be unique enough to be helpful but common enough to
@@ -594,6 +600,38 @@ class IndirectReturnOutNode extends Node {
   int getIndirectionIndex() { result = indirectionIndex }
 }
 
+private PointerType getGLValueType(Type t, int indirectionIndex) {
+  result.getBaseType() = stripPointers(t, indirectionIndex - 1)
+}
+
+bindingset[isGLValue]
+private DataFlowType getTypeImpl(Type t, int indirectionIndex, boolean isGLValue) {
+  if isGLValue = true
+  then
+    result = getGLValueType(t, indirectionIndex)
+    or
+    // Ideally, the above case would cover all glvalue cases. However, consider the case where
+    // the database consists only of:
+    // ```
+    // void test() {
+    //   int* x;
+    //   x = nullptr;
+    // }
+    // ```
+    // and we want to compute the type of `*x` in the assignment `x = nullptr`. Here, `x` is an lvalue
+    // of type int* (which morally is an int**). So when we call `getTypeImpl` it will be with the
+    // parameters:
+    // - t = int*
+    // - indirectionIndex = 1 (when we want to model the dataflow node corresponding to *x)
+    // - isGLValue = true
+    // In this case, `getTypeImpl(t, indirectionIndex, isGLValue)` should give back `int**`. In this
+    // case, however, `int**` does not exist in the database. So instead we return int* (which is
+    // wrong, but at least we have a type).
+    not exists(getGLValueType(t, indirectionIndex)) and
+    result = stripPointers(t, indirectionIndex - 1)
+  else result = stripPointers(t, indirectionIndex)
+}
+
 /**
  * INTERNAL: Do not use.
  *
@@ -615,7 +653,11 @@ class IndirectOperand extends Node, TIndirectOperand {
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }
 
-  override IRType getType() { result = this.getOperand().getIRType() }
+  override DataFlowType getType() {
+    exists(boolean isGLValue | if operand.isGLValue() then isGLValue = true else isGLValue = false |
+      result = getTypeImpl(operand.getType().getUnspecifiedType(), indirectionIndex, isGLValue)
+    )
+  }
 
   final override Location getLocationImpl() { result = this.getOperand().getLocation() }
 
@@ -645,7 +687,11 @@ class IndirectInstruction extends Node, TIndirectInstruction {
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }
 
-  override IRType getType() { result = this.getInstruction().getResultIRType() }
+  override DataFlowType getType() {
+    exists(boolean isGLValue | if instr.isGLValue() then isGLValue = true else isGLValue = false |
+      result = getTypeImpl(instr.getResultType().getUnspecifiedType(), indirectionIndex, isGLValue)
+    )
+  }
 
   final override Location getLocationImpl() { result = this.getInstruction().getLocation() }
 
@@ -859,6 +905,8 @@ abstract class PostUpdateNode extends Node {
    * Gets the node before the state update.
    */
   abstract Node getPreUpdateNode();
+
+  final override DataFlowType getType() { result = this.getPreUpdateNode().getType() }
 }
 
 /**
@@ -922,7 +970,7 @@ class VariableNode extends Node, TVariableNode {
     result = v
   }
 
-  override IRType getType() { result.getCanonicalLanguageType().hasUnspecifiedType(v.getType(), _) }
+  override DataFlowType getType() { result = v.getType() }
 
   final override Location getLocationImpl() { result = v.getLocation() }
 
@@ -1075,7 +1123,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
       store.getDestinationAddressOperand() = address
     )
     or
-    Ssa::outNodeHasAddressAndIndex(nodeFrom, address, indirectionIndex - 1)
+    Ssa::outNodeHasAddressAndIndex(nodeFrom, address, indirectionIndex)
   )
 }
 

cpp/ql/lib/experimental/semmle/code/cpp/ir/dataflow/internal/ModelUtil.qll

@@ -41,7 +41,7 @@ Node callOutput(CallInstruction call, FunctionOutput output) {
   // The side effect of a call on the value pointed to by an argument or qualifier
   exists(int index, int indirectionIndex |
     result.(IndirectArgumentOutNode).getArgumentIndex() = index and
-    result.(IndirectArgumentOutNode).getIndirectionIndex() + 1 = indirectionIndex and
+    result.(IndirectArgumentOutNode).getIndirectionIndex() = indirectionIndex and
     result.(IndirectArgumentOutNode).getCallInstruction() = call and
     output.isParameterDerefOrQualifierObject(index, indirectionIndex)
   )

cpp/ql/lib/experimental/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

@@ -11,7 +11,9 @@ private import DataFlowUtil
  * corresponding `(Indirect)OperandNode`.
  */
 predicate ignoreOperand(Operand operand) {
-  operand = any(Instruction instr | ignoreInstruction(instr)).getAnOperand()
+  operand = any(Instruction instr | ignoreInstruction(instr)).getAnOperand() or
+  operand = any(Instruction instr | ignoreInstruction(instr)).getAUse() or
+  operand instanceof MemoryOperand
 }
 
 /**

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

@@ -36,7 +36,7 @@ private module SourceVariables {
 
     override string toString() { result = var.toString() }
 
-    override DataFlowType getType() { result = var.getIRType() }
+    override DataFlowType getType() { result = var.getType() }
   }
 
   class BaseCallVariable extends BaseSourceVariable, TBaseCallVariable {
@@ -48,7 +48,7 @@ private module SourceVariables {
 
     override string toString() { result = call.toString() }
 
-    override DataFlowType getType() { result = call.getResultIRType() }
+    override DataFlowType getType() { result = call.getResultType() }
   }
 
   private newtype TSourceVariable =