C++: Encapsulate skipSkippableInstructions in a module.

Author: MathiasVP

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

@@ -240,7 +240,7 @@ private predicate instrToFieldNodeStoreStepNoChi(
   exists(StoreInstruction store, PartialFieldDefinition pd |
     pd = node2.getPartialDefinition() and
     not exists(ChiInstruction chi | chi.getPartial() = store) and
-    pd.getPreUpdateNode() = getFieldNodeForFieldInstruction(store.getDestinationAddress()) and
+    pd.getPreUpdateNode() = GetFieldNode::fromInstruction(store.getDestinationAddress()) and
     store.getSourceValueOperand() = node1.asOperand() and
     f.getADirectField() = pd.getPreUpdateNode().getField()
   )
@@ -262,7 +262,7 @@ private predicate instrToFieldNodeStoreStepChi(
     node1.asOperand() = operand and
     chi.getPartialOperand() = operand and
     store = operand.getDef() and
-    pd.getPreUpdateNode() = getFieldNodeForFieldInstruction(store.getDestinationAddress()) and
+    pd.getPreUpdateNode() = GetFieldNode::fromInstruction(store.getDestinationAddress()) and
     f.getADirectField() = pd.getPreUpdateNode().getField()
   )
 }
@@ -277,7 +277,7 @@ private predicate callableWithoutDefinitionStoreStep(
     chi.getPartial() = write and
     not chi.isResultConflated() and
     pd = node2.getPartialDefinition() and
-    pd.getPreUpdateNode() = getFieldNodeForFieldInstruction(write.getDestinationAddress()) and
+    pd.getPreUpdateNode() = GetFieldNode::fromInstruction(write.getDestinationAddress()) and
     f.getADirectField() = pd.getPreUpdateNode().getField() and
     call = write.getPrimaryInstruction() and
     callable = call.getStaticCallTarget() and
@@ -346,24 +346,6 @@ private class ArrayToPointerConvertInstruction extends ConvertInstruction {
   }
 }
 
-/**
- * These two predicates look like copy-paste from the two predicates with the same name in DataFlowUtil,
- * but crucially they only skip past `CopyValueInstruction`s. This is because we use a special case of
- * a `ConvertInstruction` to detect some read steps from arrays that undergoes array-to-pointer
- * conversion.
- */
-private Instruction skipOneCopyValueInstructionRec(CopyValueInstruction copy) {
-  copy.getUnary() = result and not result instanceof CopyValueInstruction
-  or
-  result = skipOneCopyValueInstructionRec(copy.getUnary())
-}
-
-private Instruction skipCopyValueInstructions(Operand op) {
-  not result instanceof CopyValueInstruction and result = op.getDef()
-  or
-  result = skipOneCopyValueInstructionRec(op.getDef())
-}
-
 private class InexactLoadOperand extends LoadOperand {
   InexactLoadOperand() { this.isDefinitionInexact() }
 }
@@ -407,12 +389,12 @@ private predicate instrToFieldNodeReadStep(FieldNode node1, FieldContent f, Node
     (
       exists(LoadInstruction load |
         node2.asInstruction() = load and
-        node1 = getFieldNodeForFieldInstruction(load.getSourceAddress())
+        node1 = GetFieldNode::fromInstruction(load.getSourceAddress())
       )
       or
       exists(ReadSideEffectInstruction read |
         node2.asOperand() = read.getSideEffectOperand() and
-        node1 = getFieldNodeForFieldInstruction(read.getArgumentDef())
+        node1 = GetFieldNode::fromInstruction(read.getArgumentDef())
       )
     )
   ) and
@@ -424,7 +406,7 @@ private int unbindInt(int i) { i <= result and i >= result }
 
 pragma[noinline]
 private FieldNode getFieldNodeFromLoadOperand(LoadOperand loadOperand) {
-  result = getFieldNodeForFieldInstruction(loadOperand.getAddressOperand().getDef())
+  result = GetFieldNode::fromOperand(loadOperand.getAddressOperand())
 }
 
 /**

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

@@ -185,54 +185,66 @@ class OperandNode extends Node, TOperandNode {
   override string toString() { result = this.getOperand().toString() }
 }
 
-/** An abstract class that defines conversion-like instructions. */
-abstract private class SkippableInstruction extends Instruction {
-  abstract Instruction getSourceInstruction();
-}
-
 /**
- * Gets the instruction that is propaged through a non-empty sequence of conversion-like instructions.
+ * INTERNAL: do not use. Encapsulates the details of getting a `FieldNode` from
+ * an `Instruction` or an `Operand`.
  */
-private Instruction skipSkippableInstructionsRec(SkippableInstruction skip) {
-  result = skip.getSourceInstruction() and not result instanceof SkippableInstruction
-  or
-  result = skipSkippableInstructionsRec(skip.getSourceInstruction())
-}
+module GetFieldNode {
+  /** An abstract class that defines conversion-like instructions. */
+  abstract private class SkippableInstruction extends Instruction {
+    abstract Instruction getSourceInstruction();
+  }
 
-/**
- * Gets the instruction that is propagated through a (possibly empty) sequence of conversion-like
- * instructions.
- */
-private Instruction skipSkippableInstructions(Instruction instr) {
-  result = instr and not result instanceof SkippableInstruction
-  or
-  result = skipSkippableInstructionsRec(instr)
-}
+  /**
+   * Gets the instruction that is propaged through a non-empty sequence of conversion-like instructions.
+   */
+  private Instruction skipSkippableInstructionsRec(SkippableInstruction skip) {
+    result = skip.getSourceInstruction() and not result instanceof SkippableInstruction
+    or
+    result = skipSkippableInstructionsRec(skip.getSourceInstruction())
+  }
 
-private class SkippableCopyValueInstruction extends SkippableInstruction, CopyValueInstruction {
-  override Instruction getSourceInstruction() { result = this.getSourceValue() }
-}
+  /**
+   * Gets the instruction that is propagated through a (possibly empty) sequence of conversion-like
+   * instructions.
+   */
+  private Instruction skipSkippableInstructions(Instruction instr) {
+    result = instr and not result instanceof SkippableInstruction
+    or
+    result = skipSkippableInstructionsRec(instr)
+  }
 
-private class SkippableConvertInstruction extends SkippableInstruction, ConvertInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
+  private class SkippableCopyValueInstruction extends SkippableInstruction, CopyValueInstruction {
+    override Instruction getSourceInstruction() { result = this.getSourceValue() }
+  }
 
-private class SkippableCheckedConvertInstruction extends SkippableInstruction,
-  CheckedConvertOrNullInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
+  private class SkippableConvertInstruction extends SkippableInstruction, ConvertInstruction {
+    override Instruction getSourceInstruction() { result = this.getUnary() }
+  }
 
-private class SkippableInheritanceConversionInstruction extends SkippableInstruction,
-  InheritanceConversionInstruction {
-  override Instruction getSourceInstruction() { result = this.getUnary() }
-}
+  private class SkippableCheckedConvertInstruction extends SkippableInstruction,
+    CheckedConvertOrNullInstruction {
+    override Instruction getSourceInstruction() { result = this.getUnary() }
+  }
 
-/**
- * INTERNAL: do not use. Gets the `FieldNode` corresponding to `instr`, if
- * `instr` is an instruction that propagates an address of a `FieldAddressInstruction`.
- */
-FieldNode getFieldNodeForFieldInstruction(Instruction instr) {
-  result.getFieldInstruction() = skipSkippableInstructions(instr)
+  private class SkippableInheritanceConversionInstruction extends SkippableInstruction,
+    InheritanceConversionInstruction {
+    override Instruction getSourceInstruction() { result = this.getUnary() }
+  }
+
+  /**
+   * INTERNAL: do not use. Gets the `FieldNode` corresponding to `instr`, if
+   * `instr` is an instruction that propagates an address of a `FieldAddressInstruction`.
+   */
+  FieldNode fromInstruction(Instruction instr) {
+    result.getFieldInstruction() = skipSkippableInstructions(instr)
+  }
+
+  /**
+   * INTERNAL: do not use. Gets the `FieldNode` corresponding to `op`, if the definition
+   * of `op` is an instruction that propagates an address of a `FieldAddressInstruction`.
+   */
+  FieldNode fromOperand(Operand op) { result = fromInstruction(op.getDef()) }
 }
 
 /**
@@ -265,7 +277,7 @@ class FieldNode extends Node, TFieldNode {
    * gives the `FieldNode` of `b`, and `f.getObjectNode().getObjectNode()` has no result as `a` is
    * not a field.
    */
-  FieldNode getObjectNode() { result = getFieldNodeForFieldInstruction(field.getObjectAddress()) }
+  FieldNode getObjectNode() { result = GetFieldNode::fromInstruction(field.getObjectAddress()) }
 
   /**
    * Gets the `FieldNode` that has this `FieldNode` as parent, if any.
@@ -683,7 +695,7 @@ private predicate flowOutOfPostUpdate(PartialDefinitionNode nodeFrom, Node nodeT
   exists(AddressOperand addressOperand, PartialFieldDefinition pd |
     pd = nodeFrom.getPartialDefinition() and
     not exists(pd.getPreUpdateNode().getObjectNode()) and
-    pd.getPreUpdateNode().getNextNode*() = getFieldNodeForFieldInstruction(addressOperand.getDef()) and
+    pd.getPreUpdateNode().getNextNode*() = GetFieldNode::fromOperand(addressOperand) and
     (
       exists(ChiInstruction chi |
         nodeTo.asInstruction() = chi and
@@ -711,7 +723,7 @@ private predicate flowOutOfPostUpdate(PartialDefinitionNode nodeFrom, Node nodeT
  */
 private FieldNode getOutermostFieldNode(Instruction address) {
   not exists(result.getObjectNode()) and
-  result.getNextNode*() = getFieldNodeForFieldInstruction(address)
+  result.getNextNode*() = GetFieldNode::fromInstruction(address)
 }
 
 private predicate flowIntoReadNode(Node nodeFrom, FieldNode nodeTo) {