C++: Unify partial def and def-by-ref

This removes a lot of flow steps, but it all seems to be flow that was
present twice: both exiting a `PartialDefNode` and a
`DefinitionByReferenceNode`. All `DefinitionByReferenceNode`s are now
`PartialDefNode`s.

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

@@ -13,9 +13,6 @@ private newtype TNode =
   TPostConstructorCallNode(ConstructorCall call) or
   TExplicitParameterNode(Parameter p) { exists(p.getFunction().getBlock()) } or
   TInstanceParameterNode(MemberFunction f) { exists(f.getBlock()) and not f.isStatic() } or
-  TDefinitionByReferenceNode(VariableAccess va, Expr argument) {
-    definitionByReference(va, argument)
-  } or
   TUninitializedNode(LocalVariable v) { not v.hasInitializer() }
 
 /**
@@ -152,12 +149,18 @@ class ImplicitParameterNode extends ParameterNode, TInstanceParameterNode {
  * `DefinitionByReferenceNode` to represent the value of `x` after the call has
  * returned. This node will have its `getArgument()` equal to `&x`.
  */
-class DefinitionByReferenceNode extends Node, TDefinitionByReferenceNode {
+class DefinitionByReferenceNode extends PartialDefNode {
   VariableAccess va;
 
   Expr argument;
 
-  DefinitionByReferenceNode() { this = TDefinitionByReferenceNode(va, argument) }
+  DefinitionByReferenceNode() {
+    exists(DefinitionByReference def |
+      def = this.getPartialDefinition() and
+      argument = def.getDefinedExpr() and
+      va = def.getVariableAccess()
+    )
+  }
 
   override Function getFunction() { result = va.getEnclosingFunction() }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll

@@ -93,27 +93,20 @@ private module PartialDefinitions {
       isInstanceFieldWrite(fa) and qualifier = fa.getQualifier()
     } or
     TExplicitCallQualifier(Expr qualifier, Call call) { qualifier = call.getQualifier() } or
-    TReferenceArgument(Expr arg, Call call, int i) { isReferenceOrPointerArg(arg, call, i) }
+    TReferenceArgument(Expr arg, VariableAccess va) { definitionByReference(va, arg) }
 
   private predicate isInstanceFieldWrite(FieldAccess fa) {
     not fa.getTarget().isStatic() and
     assignmentLikeOperation(_, fa.getTarget(), fa, _)
   }
 
-  private predicate isReferenceOrPointerArg(Expr arg, Call call, int i) {
-    arg = call.getArgument(i) and
-    exists(Type t | t = arg.getFullyConverted().getType().getUnspecifiedType() |
-      t instanceof ReferenceType or t instanceof PointerType
-    )
-  }
-
   class PartialDefinition extends TPartialDefinition {
     Expr definedExpr;
 
     PartialDefinition() {
       this = TExplicitFieldStoreQualifier(definedExpr, _) or
       this = TExplicitCallQualifier(definedExpr, _) or
-      this = TReferenceArgument(definedExpr, _, _)
+      this = TReferenceArgument(definedExpr, _)
     }
 
     predicate partiallyDefines(Variable v) { definedExpr = v.getAnAccess() }
@@ -135,6 +128,22 @@ private module PartialDefinitions {
 
     string toString() { result = "partial def of " + definedExpr }
   }
+
+  /**
+   * A partial definition that's a definition by reference (in the sense of the
+   * `definitionByReference` predicate).
+   */
+  class DefinitionByReference extends PartialDefinition, TReferenceArgument {
+    VariableAccess va;
+
+    DefinitionByReference() { definitionByReference(va, definedExpr) }
+
+    VariableAccess getVariableAccess() { result = va }
+
+    override predicate partiallyDefines(Variable v) { va = v.getAnAccess() }
+
+    override ControlFlowNode getSubBasicBlockStart() { result = va }
+  }
 }
 import PartialDefinitions
 private import FlowVar_internal
@@ -159,13 +168,8 @@ module FlowVar_internal {
    */
   predicate fullySupportedSsaVariable(Variable v) {
     v = any(SsaDefinition def).getAVariable() and
-    // After `foo(&x.field)` we need for there to be two definitions of `x`:
-    // the one that existed before the call to `foo` and the def-by-ref from
-    // the call. It's fundamental in SSA that each use is only associated with
-    // one def, so we can't easily get this effect with SSA.
-    not definitionByReference(v.getAnAccess(), _) and
     // A partially-defined variable is handled using the partial definitions logic.
-    not any(PartialDefinitions::PartialDefinition p).partiallyDefines(v) and
+    not any(PartialDefinition p).partiallyDefines(v) and
     // SSA variables do not exist before their first assignment, but one
     // feature of this data flow library is to track where uninitialized data
     // ends up.
@@ -213,10 +217,7 @@ module FlowVar_internal {
         or
         assignmentLikeOperation(sbb, v, _, _)
         or
-        blockVarDefinedByReference(sbb, v, _)
+        sbb = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart()
-        or
-        sbb = any(PartialDefinitions::PartialDefinition p | p.partiallyDefines(v))
-              .getSubBasicBlockStart()
         or
         blockVarDefinedByVariable(sbb, v)
       )
@@ -328,11 +329,15 @@ module FlowVar_internal {
     }
 
     override predicate definedByReference(Expr arg) {
-      blockVarDefinedByReference(sbb, v, arg)
+      exists(DefinitionByReference def |
+        def.partiallyDefines(v) and
+        sbb = def.getSubBasicBlockStart() and
+        arg = def.getDefinedExpr()
+      )
     }
 
     override predicate definedPartiallyAt(Expr e) {
-      exists(PartialDefinitions::PartialDefinition p |
+      exists(PartialDefinition p |
         p.partiallyDefines(v) and
         sbb = p.getSubBasicBlockStart() and
         e = p.getDefinedExpr()
@@ -632,11 +637,6 @@ module FlowVar_internal {
     )
   }
 
-  predicate blockVarDefinedByReference(ControlFlowNode node, Variable v, Expr argument) {
-    node = v.getAnAccess() and
-    definitionByReference(node, argument)
-  }
-
   /**
    * Holds if `v` is initialized by `init` to have value `assignedExpr`.
    */
@@ -660,10 +660,7 @@ module FlowVar_internal {
       |
         assignmentLikeOperation(this, v, _, _)
         or
-        blockVarDefinedByReference(this, v, _)
+        this = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart()
-        or
-        this = any(PartialDefinitions::PartialDefinition p | p.partiallyDefines(v))
-              .getSubBasicBlockStart()
         // It is not necessary to cut the basic blocks at `Initializer` nodes
         // because the affected variable can have no _other_ value before its
         // initializer. It is not necessary to cut basic blocks at procedure

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

@@ -42,7 +42,7 @@
 | test.cpp:431:12:431:13 | 0 | test.cpp:432:33:432:35 | tmp |
 | test.cpp:431:12:431:13 | 0 | test.cpp:433:8:433:10 | tmp |
 | test.cpp:432:10:432:13 | & ... | test.cpp:432:3:432:8 | call to memcpy |
-| test.cpp:432:10:432:13 | & ... [post update] | test.cpp:432:3:432:8 | call to memcpy |
+| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:3:432:8 | call to memcpy |
 | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:33:432:35 | tmp |
 | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:433:8:433:10 | tmp |
 | test.cpp:432:17:432:23 | source1 | test.cpp:432:10:432:13 | ref arg & ... |
@@ -54,7 +54,7 @@
 | test.cpp:437:12:437:13 | 0 | test.cpp:440:8:440:10 | tmp |
 | test.cpp:437:12:437:13 | 0 | test.cpp:442:10:442:12 | tmp |
 | test.cpp:439:10:439:13 | & ... | test.cpp:439:3:439:8 | call to memcpy |
-| test.cpp:439:10:439:13 | & ... [post update] | test.cpp:439:3:439:8 | call to memcpy |
+| test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:3:439:8 | call to memcpy |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:33:439:35 | tmp |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:440:8:440:10 | tmp |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:442:10:442:12 | tmp |

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

@@ -24,12 +24,7 @@ edges
 | A.cpp:103:14:103:14 | c [a, ... (1)] | A.cpp:120:12:120:13 | c1 [a, ... (1)] |
 | A.cpp:107:12:107:13 | c1 [a, ... (1)] | A.cpp:107:16:107:16 | a |
 | A.cpp:120:12:120:13 | c1 [a, ... (1)] | A.cpp:120:16:120:16 | a |
-| A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | A.cpp:131:8:131:8 | b [post update] [c, ... (1)] |
-| A.cpp:126:12:126:18 | new [void] | A.cpp:126:5:126:5 | b [post update] [c, ... (1)] |
-| A.cpp:131:8:131:8 | b [post update] [c, ... (1)] | A.cpp:132:10:132:10 | b [c, ... (1)] |
-| A.cpp:132:10:132:10 | b [c, ... (1)] | A.cpp:132:13:132:13 | c |
 | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:143:7:143:31 | ... = ... [c, ... (1)] |
-| A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:151:18:151:18 | b [post update] [c, ... (1)] |
 | A.cpp:142:7:142:20 | ... = ... [void] | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] |
 | A.cpp:142:14:142:20 | new [void] | A.cpp:142:7:142:20 | ... = ... [void] |
 | A.cpp:143:7:143:10 | this [post update] [b, ... (1)] | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] |
@@ -40,12 +35,10 @@ edges
 | A.cpp:150:12:150:18 | new [void] | A.cpp:151:18:151:18 | b [void] |
 | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] | A.cpp:152:10:152:10 | d [b, ... (1)] |
 | A.cpp:151:12:151:24 | call to D [post update] [b, ... (2)] | A.cpp:153:10:153:10 | d [b, ... (2)] |
-| A.cpp:151:18:151:18 | b [post update] [c, ... (1)] | A.cpp:154:10:154:10 | b [c, ... (1)] |
 | A.cpp:151:18:151:18 | b [void] | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] |
 | A.cpp:152:10:152:10 | d [b, ... (1)] | A.cpp:152:13:152:13 | b |
 | A.cpp:153:10:153:10 | d [b, ... (2)] | A.cpp:153:13:153:13 | b [c, ... (1)] |
 | A.cpp:153:13:153:13 | b [c, ... (1)] | A.cpp:153:16:153:16 | c |
-| A.cpp:154:10:154:10 | b [c, ... (1)] | A.cpp:154:13:154:13 | c |
 | A.cpp:159:12:159:18 | new [void] | A.cpp:160:29:160:29 | b [void] |
 | A.cpp:160:18:160:60 | call to MyList [post update] [head, ... (1)] | A.cpp:161:38:161:39 | l1 [head, ... (1)] |
 | A.cpp:160:29:160:29 | b [void] | A.cpp:160:18:160:60 | call to MyList [post update] [head, ... (1)] |
@@ -114,11 +107,9 @@ edges
 | A.cpp:75:14:75:14 | c | A.cpp:73:25:73:32 | new [void] | A.cpp:75:14:75:14 | c | c flows from $@ | A.cpp:73:25:73:32 | new [void] | new [void] |
 | A.cpp:107:16:107:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:107:16:107:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] |
 | A.cpp:120:16:120:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:120:16:120:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] |
-| A.cpp:132:13:132:13 | c | A.cpp:126:12:126:18 | new [void] | A.cpp:132:13:132:13 | c | c flows from $@ | A.cpp:126:12:126:18 | new [void] | new [void] |
 | A.cpp:152:13:152:13 | b | A.cpp:143:25:143:31 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:143:25:143:31 | new [void] | new [void] |
 | A.cpp:152:13:152:13 | b | A.cpp:150:12:150:18 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:150:12:150:18 | new [void] | new [void] |
 | A.cpp:153:16:153:16 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:153:16:153:16 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] |
-| A.cpp:154:13:154:13 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:154:13:154:13 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] |
 | A.cpp:165:26:165:29 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:165:26:165:29 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] |
 | A.cpp:169:15:169:18 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:169:15:169:18 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] |
 | B.cpp:9:20:9:24 | elem1 | B.cpp:6:15:6:24 | new [void] | B.cpp:9:20:9:24 | elem1 | elem1 flows from $@ | B.cpp:6:15:6:24 | new [void] | new [void] |

cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

@@ -140,24 +140,17 @@
 | taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer |  |
 | taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT |
-| taint.cpp:168:8:168:14 | tainted [post update] | taint.cpp:172:18:172:24 | tainted |  |
 | taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy |  |
-| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:170:3:170:8 | call to strcpy |  |
+| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:3:170:8 | call to strcpy |  |
-| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:171:8:171:13 | buffer |  |
-| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:172:10:172:15 | buffer |  |
-| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer |  |
 | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer |  |
 | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:170:18:170:26 | Hello,  | taint.cpp:170:10:170:15 | ref arg buffer | TAINT |
-| taint.cpp:171:8:171:13 | buffer [post update] | taint.cpp:172:10:172:15 | buffer |  |
-| taint.cpp:171:8:171:13 | buffer [post update] | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:172:10:172:15 | buffer |  |
 | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat |  |
 | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:10:172:15 | ref arg buffer | TAINT |
-| taint.cpp:172:10:172:15 | buffer [post update] | taint.cpp:172:3:172:8 | call to strcat |  |
+| taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:172:3:172:8 | call to strcat |  |
-| taint.cpp:172:10:172:15 | buffer [post update] | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:172:18:172:24 | tainted | taint.cpp:172:10:172:15 | ref arg buffer | TAINT |
 | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p |  |
@@ -168,7 +161,7 @@
 | taint.cpp:193:6:193:6 | x | taint.cpp:194:10:194:10 | x |  |
 | taint.cpp:193:6:193:6 | x | taint.cpp:195:7:195:7 | x |  |
 | taint.cpp:194:9:194:10 | & ... | taint.cpp:194:2:194:7 | call to memcpy |  |
-| taint.cpp:194:9:194:10 | & ... [post update] | taint.cpp:194:2:194:7 | call to memcpy |  |
+| taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:194:2:194:7 | call to memcpy |  |
 | taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:195:7:195:7 | x |  |
 | taint.cpp:194:10:194:10 | x | taint.cpp:194:9:194:10 | & ... | TAINT |
 | taint.cpp:194:13:194:18 | source | taint.cpp:194:9:194:10 | ref arg & ... | TAINT |
@@ -183,7 +176,5 @@
 | taint.cpp:208:6:208:6 | 0 | taint.cpp:216:7:216:7 | y |  |
 | taint.cpp:213:12:213:12 | ref arg x | taint.cpp:215:7:215:7 | x |  |
 | taint.cpp:213:12:213:12 | x | taint.cpp:213:15:213:15 | ref arg y |  |
-| taint.cpp:213:12:213:12 | x [post update] | taint.cpp:215:7:215:7 | x |  |
 | taint.cpp:213:15:213:15 | ref arg y | taint.cpp:216:7:216:7 | y |  |
 | taint.cpp:213:15:213:15 | y | taint.cpp:213:12:213:12 | ref arg x |  |
-| taint.cpp:213:15:213:15 | y [post update] | taint.cpp:216:7:216:7 | y |  |