Merge pull request github#3785 from MathiasVP/dataflow-operand-nodes

C++: Operands as dataflow nodes

Author: rdmarsh2

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

@@ -303,4 +303,4 @@ predicate isImmutableOrUnobservable(Node n) {
 }
 
 /** Holds if `n` should be hidden from path explanations. */
-predicate nodeIsHidden(Node n) { none() }
+predicate nodeIsHidden(Node n) { n instanceof OperandNode }

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

@@ -13,6 +13,7 @@ private import semmle.code.cpp.models.interfaces.DataFlow
 
 private newtype TIRDataFlowNode =
   TInstructionNode(Instruction i) or
+  TOperandNode(Operand op) or
   TVariableNode(Variable var)
 
 /**
@@ -37,6 +38,9 @@ class Node extends TIRDataFlowNode {
   /** Gets the instruction corresponding to this node, if any. */
   Instruction asInstruction() { result = this.(InstructionNode).getInstruction() }
 
+  /** Gets the operands corresponding to this node, if any. */
+  Operand asOperand() { result = this.(OperandNode).getOperand() }
+
   /**
    * Gets the non-conversion expression corresponding to this node, if any. If
    * this node strictly (in the sense of `asConvertedExpr`) corresponds to a
@@ -132,6 +136,28 @@ class InstructionNode extends Node, TInstructionNode {
   }
 }
 
+/**
+ * An operand, viewed as a node in a data flow graph.
+ */
+class OperandNode extends Node, TOperandNode {
+  Operand op;
+
+  OperandNode() { this = TOperandNode(op) }
+
+  /** Gets the operand corresponding to this node. */
+  Operand getOperand() { result = op }
+
+  override Declaration getEnclosingCallable() { result = this.getFunction() }
+
+  override Function getFunction() { result = op.getUse().getEnclosingFunction() }
+
+  override Type getType() { result = op.getType() }
+
+  override Location getLocation() { result = op.getLocation() }
+
+  override string toString() { result = this.getOperand().toString() }
+}
+
 /**
  * An expression, viewed as a node in a data flow graph.
  */
@@ -291,7 +317,7 @@ abstract class PostUpdateNode extends InstructionNode {
  * setY(&x); // a partial definition of the object `x`.
  * ```
  */
-abstract private class PartialDefinitionNode extends PostUpdateNode, TInstructionNode {
+abstract private class PartialDefinitionNode extends PostUpdateNode {
   abstract Expr getDefinedExpr();
 }
 
@@ -306,11 +332,11 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
     )
   }
 
-  // There might be multiple `ChiInstructions` that has a particular instruction as
-  // the total operand - so this definition gives consistency errors in
-  // DataFlowImplConsistency::Consistency. However, it's not clear what (if any) implications
-  // this consistency failure has.
-  override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
+  // By using an operand as the result of this predicate we avoid the dataflow inconsistency errors
+  // caused by having multiple nodes sharing the same pre update node. This inconsistency error can cause
+  // a tuple explosion in the big step dataflow relation since it can make many nodes be the entry node
+  // into a big step.
+  override Node getPreUpdateNode() { result.asOperand() = instr.getTotalOperand() }
 
   override Expr getDefinedExpr() {
     result = field.getObjectAddress().getUnconvertedResultExpression()
@@ -482,7 +508,11 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFr
  * data flow. It may have less flow than the `localFlowStep` predicate.
  */
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
+  // Instruction -> Instruction flow
   simpleInstructionLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+  or
+  // Operand -> Instruction flow
+  simpleOperandLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
 }
 
 pragma[noinline]
@@ -494,6 +524,16 @@ private predicate getFieldSizeOfClass(Class c, Type type, int size) {
   )
 }
 
+private predicate simpleOperandLocalFlowStep(Operand opFrom, Instruction iTo) {
+  // Certain dataflow steps (for instance `PostUpdateNode.getPreUpdateNode()`) generates flow to
+  // operands, so we include dataflow from those operands to the "result" of the instruction (i.e., to
+  // the instruction itself).
+  exists(PostUpdateNode post |
+    opFrom = post.getPreUpdateNode().asOperand() and
+    iTo.getAnOperand() = opFrom
+  )
+}
+
 cached
 private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) {
   iTo.(CopyInstruction).getSourceValue() = iFrom

cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected

@@ -27,8 +27,6 @@ localCallNodes
 postIsNotPre
 postHasUniquePre
 uniquePostUpdate
-| ref.cpp:83:5:83:17 | Chi | Node has multiple PostUpdateNodes. |
-| ref.cpp:109:5:109:22 | Chi | Node has multiple PostUpdateNodes. |
 postIsInSameCallable
 reverseRead
 storeIsPostUpdate