Merge pull request #12141 from MathiasVP/fix-multiple-out-nodes

C++: Deduplicate `OutNode`s
2025-12-23 20:26:32 +01:00 · 2023-02-10 08:39:16 +00:00
parent 9d6098af15 4719fd5235
commit 9aa9187dd7
6 changed files with 86 additions and 39 deletions
--- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
@@ -404,7 +404,7 @@ class ReturnIndirectionNode extends IndirectReturnNode, ReturnNode {
  }
 }

-private Operand fullyConvertedCallStep(Operand op) {
+private Operand fullyConvertedCallStepImpl(Operand op) {
  not exists(getANonConversionUse(op)) and
  exists(Instruction instr |
    conversionFlow(op, instr, _, _) and
@@ -412,6 +412,10 @@ private Operand fullyConvertedCallStep(Operand op) {
  )
 }

+private Operand fullyConvertedCallStep(Operand op) {
+  result = unique( | | fullyConvertedCallStepImpl(op))
+}
+
 /**
 * Gets the instruction that uses this operand, if the instruction is not
 * ignored for dataflow purposes.
@@ -438,10 +442,21 @@ private Instruction getANonConversionUse(Operand operand) {
 }

 /**
- * Gets the operand that represents the first use of the value of `call` following
+ * Gets an operand that represents the use of the value of `call` following
 * a sequence of conversion-like instructions.
+ *
+ * Note that `operand` is not functionally determined by `call` since there
+ * can be multiple sequences of disjoint conversions following a call. For example,
+ * consider an example like:
+ * ```cpp
+ * long f();
+ * int y;
+ * long x = (long)(y = (int)f());
+ * ```
+ * in this case, there'll be a long-to-int conversion on `f()` before the value is assigned to `y`,
+ * and there will be an int-to-long conversion on `(int)f()` before the value is assigned to `x`.
 */
-predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) {
+private predicate operandForFullyConvertedCallImpl(Operand operand, CallInstruction call) {
  exists(getANonConversionUse(operand)) and
  (
    operand = getAUse(call)
@@ -450,6 +465,24 @@ predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) {
  )
 }

+/**
+ * Gets the operand that represents the use of the value of `call` following
+ * a sequence of conversion-like instructions, if a unique operand exists.
+ */
+predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) {
+  operand = unique(Operand cand | operandForFullyConvertedCallImpl(cand, call))
+}
+
+private predicate instructionForFullyConvertedCallWithConversions(
+  Instruction instr, CallInstruction call
+) {
+  instr =
+    getUse(unique(Operand operand |
+        operand = fullyConvertedCallStep*(getAUse(call)) and
+        not exists(fullyConvertedCallStep(operand))
+      ))
+}
+
 /**
 * Gets the instruction that represents the first use of the value of `call` following
 * a sequence of conversion-like instructions.
@@ -458,16 +491,15 @@ predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) {
 * conversion instruction) to use to represent the value of `call` after conversions.
 */
 predicate instructionForFullyConvertedCall(Instruction instr, CallInstruction call) {
+  // Only pick an instruction for the call if we cannot pick a unique operand.
  not operandForFullyConvertedCall(_, call) and
  (
    // If there is no use of the call then we pick the call instruction
-    not exists(getAUse(call)) and
+    not instructionForFullyConvertedCallWithConversions(_, call) and
    instr = call
    or
-    // Otherwise, flow to the first non-conversion use.
-    exists(Operand operand | operand = fullyConvertedCallStep*(getAUse(call)) |
-      instr = getANonConversionUse(operand)
-    )
+    // Otherwise, flow to the first instruction that defines multiple operands.
+    instructionForFullyConvertedCallWithConversions(instr, call)
  )
 }

--- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected
+++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected
--- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql
+++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql
@@ -0,0 +1,46 @@
+import TestUtilities.InlineExpectationsTest
+import cpp
+
+module AstTest {
+  private import semmle.code.cpp.dataflow.old.DataFlow::DataFlow
+  private import semmle.code.cpp.dataflow.old.internal.DataFlowPrivate
+
+  class ASTMultipleOutNodesTest extends InlineExpectationsTest {
+    ASTMultipleOutNodesTest() { this = "ASTMultipleOutNodesTest" }
+
+    override string getARelevantTag() { result = "ast-count(" + any(ReturnKind k).toString() + ")" }
+
+    override predicate hasActualResult(Location location, string element, string tag, string value) {
+      exists(DataFlowCall call, int n, ReturnKind kind |
+        call.getLocation() = location and
+        n = strictcount(getAnOutNode(call, kind)) and
+        n > 1 and
+        element = call.toString() and
+        tag = "ast-count(" + kind.toString() + ")" and
+        value = n.toString()
+      )
+    }
+  }
+}
+
+module IRTest {
+  private import semmle.code.cpp.ir.dataflow.DataFlow
+  private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
+
+  class IRMultipleOutNodesTest extends InlineExpectationsTest {
+    IRMultipleOutNodesTest() { this = "IRMultipleOutNodesTest" }
+
+    override string getARelevantTag() { result = "ir-count(" + any(ReturnKind k).toString() + ")" }
+
+    override predicate hasActualResult(Location location, string element, string tag, string value) {
+      exists(DataFlowCall call, int n, ReturnKind kind |
+        call.getLocation() = location and
+        n = strictcount(getAnOutNode(call, kind)) and
+        n > 1 and
+        element = call.toString() and
+        tag = "ir-count(" + kind.toString() + ")" and
+        value = n.toString()
+      )
+    }
+  }
+}
--- a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
@@ -12,16 +12,6 @@ compatibleTypesReflexive
 unreachableNodeCCtx
 localCallNodes
 postIsNotPre
-| A.cpp:98:12:98:18 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| B.cpp:6:15:6:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| B.cpp:15:15:15:27 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| C.cpp:22:12:22:21 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| C.cpp:24:16:24:25 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| D.cpp:28:15:28:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| D.cpp:35:15:35:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| D.cpp:42:15:42:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| D.cpp:49:15:49:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| D.cpp:56:15:56:24 | new indirection | PostUpdateNode should not equal its pre-update node. |
 postHasUniquePre
 uniquePostUpdate
 | aliasing.cpp:70:11:70:11 | definition of w indirection | Node has multiple PostUpdateNodes. |
@@ -42,16 +32,6 @@ postIsInSameCallable
 reverseRead
 argHasPostUpdate
 postWithInFlow
-| A.cpp:98:12:98:18 | new indirection | PostUpdateNode should not be the target of local flow. |
-| B.cpp:6:15:6:24 | new indirection | PostUpdateNode should not be the target of local flow. |
-| B.cpp:15:15:15:27 | new indirection | PostUpdateNode should not be the target of local flow. |
-| C.cpp:22:12:22:21 | new indirection | PostUpdateNode should not be the target of local flow. |
-| C.cpp:24:16:24:25 | new indirection | PostUpdateNode should not be the target of local flow. |
-| D.cpp:28:15:28:24 | new indirection | PostUpdateNode should not be the target of local flow. |
-| D.cpp:35:15:35:24 | new indirection | PostUpdateNode should not be the target of local flow. |
-| D.cpp:42:15:42:24 | new indirection | PostUpdateNode should not be the target of local flow. |
-| D.cpp:49:15:49:24 | new indirection | PostUpdateNode should not be the target of local flow. |
-| D.cpp:56:15:56:24 | new indirection | PostUpdateNode should not be the target of local flow. |
 | realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 | realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
--- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
@@ -31,7 +31,6 @@ edges
 | A.cpp:57:11:57:24 | call to B [c] | A.cpp:57:11:57:24 | new indirection [c] |
 | A.cpp:57:11:57:24 | new indirection [c] | A.cpp:28:8:28:10 | this indirection [c] |
 | A.cpp:57:11:57:24 | new indirection [c] | A.cpp:57:10:57:32 | call to get |
-| A.cpp:57:11:57:24 | new indirection [c] | A.cpp:57:11:57:24 | new indirection [c] |
 | A.cpp:57:17:57:23 | new | A.cpp:23:10:23:10 | c |
 | A.cpp:57:17:57:23 | new | A.cpp:57:11:57:24 | call to B [c] |
 | A.cpp:57:17:57:23 | new | A.cpp:57:17:57:23 | new |
--- a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected
+++ b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected
@@ -223,11 +223,6 @@ compatibleTypesReflexive
 unreachableNodeCCtx
 localCallNodes
 postIsNotPre
-| condition_decls.cpp:3:13:3:22 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| defdestructordeleteexpr.cpp:4:9:4:15 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| deleteexpr.cpp:7:9:7:15 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| ir.cpp:943:3:943:11 | new indirection | PostUpdateNode should not equal its pre-update node. |
-| ir.cpp:947:3:947:25 | new indirection | PostUpdateNode should not equal its pre-update node. |
 postHasUniquePre
 uniquePostUpdate
 | cpp11.cpp:82:17:82:17 | this indirection | Node has multiple PostUpdateNodes. |
@@ -251,19 +246,14 @@ postIsInSameCallable
 reverseRead
 argHasPostUpdate
 postWithInFlow
-| condition_decls.cpp:3:13:3:22 | new indirection | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:77:19:77:21 | call to Val | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:82:11:82:14 | call to Val | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:82:45:82:48 | call to Val | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:82:51:82:51 | call to Val | PostUpdateNode should not be the target of local flow. |
-| defdestructordeleteexpr.cpp:4:9:4:15 | new indirection | PostUpdateNode should not be the target of local flow. |
-| deleteexpr.cpp:7:9:7:15 | new indirection | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:809:7:809:13 | call to Base | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:810:7:810:26 | call to Base | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:823:7:823:13 | call to Base | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:824:7:824:26 | call to Base | PostUpdateNode should not be the target of local flow. |
-| ir.cpp:943:3:943:11 | new indirection | PostUpdateNode should not be the target of local flow. |
-| ir.cpp:947:3:947:25 | new indirection | PostUpdateNode should not be the target of local flow. |
 | try_catch.cpp:7:8:7:8 | call to exception | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition