Merge pull request #11976 from MathiasVP/fewer-uses-2

C++: Don't count every conversion as a use

Author: MathiasVP

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

@@ -370,7 +370,7 @@ class ReturnIndirectionNode extends IndirectReturnNode, ReturnNode {
 private Operand fullyConvertedCallStep(Operand op) {
   not exists(getANonConversionUse(op)) and
   exists(Instruction instr |
-    conversionFlow(op, instr, _) and
+    conversionFlow(op, instr, _, _) and
     result = getAUse(instr)
   )
 }
@@ -397,7 +397,7 @@ Operand getAUse(Instruction instr) {
  */
 private Instruction getANonConversionUse(Operand operand) {
   result = getUse(operand) and
-  not conversionFlow(_, result, _)
+  not conversionFlow(_, result, _, _)
 }
 
 /**
@@ -555,7 +555,7 @@ private predicate numberOfLoadsFromOperandRec(Operand operandFrom, Operand opera
   or
   exists(Operand op, Instruction instr |
     instr = op.getDef() and
-    conversionFlow(operandFrom, instr, _) and
+    conversionFlow(operandFrom, instr, _, _) and
     numberOfLoadsFromOperand(op, operandTo, ind)
   )
 }
@@ -568,7 +568,7 @@ private predicate numberOfLoadsFromOperand(Operand operandFrom, Operand operandT
   numberOfLoadsFromOperandRec(operandFrom, operandTo, n)
   or
   not Ssa::isDereference(_, operandFrom) and
-  not conversionFlow(operandFrom, _, _) and
+  not conversionFlow(operandFrom, _, _, _) and
   operandFrom = operandTo and
   n = 0
 }

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

@@ -77,22 +77,32 @@ class FieldAddress extends Operand {
  *
  * `isPointerArith` is `true` if `instrTo` is a `PointerArithmeticInstruction` and `opFrom`
  * is the left operand.
+ *
+ * `additional` is `true` if the conversion is supplied by an implementation of the
+ * `Indirection` class. It is sometimes useful to exclude such conversions.
  */
-predicate conversionFlow(Operand opFrom, Instruction instrTo, boolean isPointerArith) {
+predicate conversionFlow(
+  Operand opFrom, Instruction instrTo, boolean isPointerArith, boolean additional
+) {
   isPointerArith = false and
   (
-    instrTo.(CopyValueInstruction).getSourceValueOperand() = opFrom
-    or
-    instrTo.(ConvertInstruction).getUnaryOperand() = opFrom
-    or
-    instrTo.(CheckedConvertOrNullInstruction).getUnaryOperand() = opFrom
-    or
-    instrTo.(InheritanceConversionInstruction).getUnaryOperand() = opFrom
+    additional = false and
+    (
+      instrTo.(CopyValueInstruction).getSourceValueOperand() = opFrom
+      or
+      instrTo.(ConvertInstruction).getUnaryOperand() = opFrom
+      or
+      instrTo.(CheckedConvertOrNullInstruction).getUnaryOperand() = opFrom
+      or
+      instrTo.(InheritanceConversionInstruction).getUnaryOperand() = opFrom
+    )
     or
+    additional = true and
     Ssa::isAdditionalConversionFlow(opFrom, instrTo)
   )
   or
   isPointerArith = true and
+  additional = false and
   instrTo.(PointerArithmeticInstruction).getLeftOperand() = opFrom
 }
 
@@ -1365,7 +1375,7 @@ private module Cached {
 
   private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
     // Treat all conversions as flow, even conversions between different numeric types.
-    conversionFlow(opFrom, iTo, false)
+    conversionFlow(opFrom, iTo, false, _)
     or
     iTo.(CopyInstruction).getSourceValueOperand() = opFrom
   }

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

@@ -469,7 +469,7 @@ private predicate indirectConversionFlowStep(Node nFrom, Node nTo) {
     hasOperandAndIndex(nFrom, op1, pragma[only_bind_into](indirectionIndex)) and
     hasOperandAndIndex(nTo, op2, pragma[only_bind_into](indirectionIndex)) and
     instr = op2.getDef() and
-    conversionFlow(op1, instr, _)
+    conversionFlow(op1, instr, _, _)
   )
 }
 

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

@@ -208,7 +208,7 @@ private module IteratorIndirections {
 
     override predicate isAdditionalDereference(Instruction deref, Operand address) {
       exists(CallInstruction call |
-        operandForfullyConvertedCall(deref.getAUse(), call) and
+        operandForfullyConvertedCall(getAUse(deref), call) and
         this = call.getStaticCallTarget().getClassAndName("operator*") and
         address = call.getThisArgumentOperand()
       )
@@ -586,6 +586,15 @@ private module Cached {
     )
   }
 
+  /** Holds if `op` is the only use of its defining instruction, and that op is used in a conversation */
+  private predicate isConversion(Operand op) {
+    exists(Instruction def, Operand use |
+      def = op.getDef() and
+      use = unique( | | getAUse(def)) and
+      conversionFlow(use, _, false, false)
+    )
+  }
+
   /**
    * Holds if `op` is a use of an SSA variable rooted at `base` with `ind` number
    * of indirections.
@@ -603,6 +612,9 @@ private module Cached {
       type = getLanguageType(op) and
       upper = countIndirectionsForCppType(type) and
       isUseImpl(op, base, ind0) and
+      // Don't count every conversion as their own use. Instead, only the first
+      // use (i.e., before any conversions are applied) will count as a use.
+      not isConversion(op) and
       ind = ind0 + [0 .. upper] and
       indirectionIndex = ind - ind0
     )
@@ -653,7 +665,7 @@ private module Cached {
     exists(Operand mid, Instruction instr |
       isUseImpl(mid, base, ind) and
       instr = operand.getDef() and
-      conversionFlow(mid, instr, false)
+      conversionFlow(mid, instr, false, _)
     )
     or
     exists(int ind0 |
@@ -723,7 +735,7 @@ private module Cached {
     exists(Operand mid, Instruction instr, boolean certain0, boolean isPointerArith |
       isDefImpl(mid, base, ind, certain0) and
       instr = operand.getDef() and
-      conversionFlow(mid, instr, isPointerArith) and
+      conversionFlow(mid, instr, isPointerArith, _) and
       if isPointerArith = true then certain = false else certain = certain0
     )
     or

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

@@ -48,5 +48,5 @@ void following_pointers(
 
   int stackArray[2] = { source(), source() };
   stackArray[0] = source();
-  sink(stackArray); // $ ast ir ir=49:35 ir=50:19
+  sink(stackArray); // $ ast ir
 }

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

@@ -533,12 +533,12 @@ void test_set_through_const_pointer(int *e)
 }
 
 void sink_then_source_1(int* p) {
-    sink(*p); // $ SPURIOUS: ir=537:10 ir=547:9
+    sink(*p); // $ ir // Flow from the unitialized x to the dereference.
     *p = source();
 }
 
 void sink_then_source_2(int* p, int y) {
-    sink(y); // $ SPURIOUS: ast ir=542:10 ir=551:9
+    sink(y); // $ SPURIOUS: ast ir
     *p = source();
 }
 

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

@@ -13,8 +13,6 @@ edges
 | A.cpp:31:20:31:20 | c | A.cpp:31:14:31:21 | call to B [c] |
 | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
 | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
-| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
-| A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection |
 | A.cpp:47:12:47:18 | new | A.cpp:48:20:48:20 | c |
 | A.cpp:48:12:48:18 | call to make indirection [c] | A.cpp:49:10:49:10 | b indirection [c] |
 | A.cpp:48:20:48:20 | c | A.cpp:29:23:29:23 | c |
@@ -907,7 +905,6 @@ nodes
 | A.cpp:41:15:41:21 | new | semmle.label | new |
 | A.cpp:41:15:41:21 | new | semmle.label | new |
 | A.cpp:43:10:43:12 | & ... indirection | semmle.label | & ... indirection |
-| A.cpp:43:10:43:12 | & ... indirection | semmle.label | & ... indirection |
 | A.cpp:47:12:47:18 | new | semmle.label | new |
 | A.cpp:48:12:48:18 | call to make indirection [c] | semmle.label | call to make indirection [c] |
 | A.cpp:48:20:48:20 | c | semmle.label | c |
@@ -1765,8 +1762,6 @@ subpaths
 #select
 | A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
 | A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
-| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
-| A.cpp:43:10:43:12 | & ... indirection | A.cpp:41:15:41:21 | new | A.cpp:43:10:43:12 | & ... indirection | & ... indirection flows from $@ | A.cpp:41:15:41:21 | new | new |
 | A.cpp:49:10:49:13 | c | A.cpp:47:12:47:18 | new | A.cpp:49:10:49:13 | c | c flows from $@ | A.cpp:47:12:47:18 | new | new |
 | A.cpp:49:13:49:13 | c | A.cpp:47:12:47:18 | new | A.cpp:49:13:49:13 | c | c flows from $@ | A.cpp:47:12:47:18 | new | new |
 | A.cpp:56:10:56:17 | call to get | A.cpp:55:12:55:19 | new | A.cpp:56:10:56:17 | call to get | call to get flows from $@ | A.cpp:55:12:55:19 | new | new |