C++: Eliminate the old way of skipping past copy value instructions in read step.

Author: MathiasVP

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

@@ -301,41 +301,36 @@ predicate storeStep(Node node1, Content c, Node node2) {
   arrayStoreStepChi(node1, c, node2)
 }
 
-private class ArrayToPointerConvertInstruction extends ConvertInstruction {
-  ArrayToPointerConvertInstruction() {
-    this.getUnary().getResultType() instanceof ArrayType and
-    this.getResultType() instanceof PointerType
-  }
-}
-
-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 predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
+private predicate arrayReadStep(Node node1, ArrayContent a, AddressNodeRead node2) {
   a = TArrayContent() and
   // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
-  exists(LoadOperand operand, Instruction address |
+  // Note that we don't register read side effects as read steps since we have dataflow from the
+  // side effect operand's definition to the side effect operand even without a read step. So we only
+  // use `LoadInstruction`s here.
+  exists(Operand operand, LoadInstruction load |
+    operand.getAnyDef() = getSourceValue(load) and
     operand.isDefinitionInexact() and
     node1.asInstruction() = operand.getAnyDef() and
-    operand = node2.asOperand() and
-    address = skipCopyValueInstructions(operand.getAddressOperand()) and
     (
-      address instanceof LoadInstruction or
-      address instanceof ArrayToPointerConvertInstruction or
-      address instanceof PointerOffsetInstruction
+      // `use(p[i])`
+      exists(PointerAddInstruction add |
+        add = getSourceAddress(load) and
+        node2.getInstruction() = add
+      )
+      or
+      // `use(*p)`
+      // Search backwards through `CopyValue` instructions and check if there's an address instruction
+      // that is not just a glvalue.
+      exists(Instruction i | i = instructionOrUnaryDefinition(getSourceAddress(load)) |
+        not i.isGLValue() and
+        i.getResultIRType() instanceof IRAddressType and
+        node2.getInstruction() = getSourceAddress(load)
+      )
     ) and
+    // Don't perform an array read step if we're already performing a field read step.
     not exists(Instruction i |
       fieldReadStep(addressNodeRead(i), _, _) and
-      AddressFlow::addressFlowInstrTC(i, operand.getUse())
+      AddressFlow::addressFlowInstrTC(i, load)
     )
   )
 }

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

@@ -39,8 +39,8 @@ void sink(int x)
 
 void bar(Outer &b)
 {
-  sink(b.inner.f.a()); // $ ast=53:19 ast=55:19 MISSING: ir
-  sink(b.inner.f.b()); // $ ast=54:19 ast=56:19 MISSING: ir
+  sink(b.inner.f.a()); // $ ast,ir=53:19 ast,ir=55:19
+  sink(b.inner.f.b()); // $ ast,ir=54:19 ast,ir=56:19
 }
 
 void foo()

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

@@ -12,7 +12,7 @@ struct Outer {
 };
 
 void absink(struct AB *ab) {
-  sink(ab->a); //$ ast=20:20 ast=27:7 ast=40:20 ir
+  sink(ab->a); //$ ast,ir=20:20 ast,ir=27:7 ast=40:20
   sink(ab->b); // no flow
 }
 

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

@@ -75,7 +75,7 @@ void test_copy_assignment_operator()
 
     y = x;
 
-    sink(y.data1); // $ ast=71:15 SPURIOUS: ast=69:23 MISSING: ir
+    sink(y.data1); // $ ast=71:15 ir SPURIOUS: ast=69:23
     sink(x.data1); // $ ast,ir
 
     IntWrapper::Class z1, z2;
@@ -84,7 +84,7 @@ void test_copy_assignment_operator()
 
     swap(z1, z2);
 
-    sink(z2.data1); // $ MISSING: ast,ir
+    sink(z2.data1); // $ ir MISSING: ast
     sink(z1.data1); // $ SPURIOUS: ir ast=81:27 ast=82:16
 }
 
@@ -99,7 +99,7 @@ void test_move_assignment_operator()
 
     y = std::move(x);
 
-    sink(y.data1); // $ ast=95:15 SPURIOUS: ast=93:23 MISSING: ir
+    sink(y.data1); // $ ast=95:15 ir SPURIOUS: ast=93:23
     sink(x.data1); // $ ast,ir
 }
 
@@ -126,7 +126,7 @@ void test_copy_assignment_method()
 
     y.copy_assign(x);
 
-    sink(y.data1); // $ ast=122:15 SPURIOUS: ast=120:23 MISSING: ir
+    sink(y.data1); // $ ast=122:15 ir SPURIOUS: ast=120:23
     sink(x.data1); // $ ast,ir
 }
 
@@ -141,7 +141,7 @@ void test_move_assignment_method()
 
     y.move_assign(std::move(x));
 
-    sink(y.data1); // $ ast=137:15 SPURIOUS: ast=135:23 MISSING: ir
+    sink(y.data1); // $ ast=137:15 ir SPURIOUS: ast=135:23
     sink(x.data1); // $ ast,ir
 }