C++: Fix Expr <-> DataFlow::Node mapping for glvalue qualifiers

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

@@ -946,6 +946,24 @@ module Public {
    */
   ExprNode exprNode(Expr e) { result.getExpr(_) = e }
 
+  /**
+   * Gets the `Node` corresponding to the value after `indirection` number of
+   * dereferences of `e` or any of its conversions. There is no result if `e`
+   * is a `Conversion`. For data flowing _out of_ an expression, like when an
+   * argument is passed by reference, use
+   * `definitionByReferenceNodeFromArgument` instead.
+   */
+  IndirectExprNode indirectExprNode(Expr e, int indirection) { result.getExpr(indirection) = e }
+
+  /**
+   * Gets the `Node` corresponding to the value after any number of
+   * dereferences of `e` or any of its conversions. There is no result if `e`
+   * is a `Conversion`. For data flowing _out of_ an expression, like when an
+   * argument is passed by reference, use
+   * `definitionByReferenceNodeFromArgument` instead.
+   */
+  IndirectExprNode indirectExprNode(Expr e) { result = indirectExprNode(e, _) }
+
   /**
    * Gets the `Node` corresponding to the value of evaluating `e`. Here, `e` may
    * be a `Conversion`. For data flowing _out of_ an expression, like when an
@@ -957,7 +975,7 @@ module Public {
   /**
    * Gets the `Node` corresponding to the value of `p` at function entry.
    */
-  ExplicitParameterNode parameterNode(Parameter p) { result.getParameter() = p }
+  ParameterNode parameterNode(Parameter p) { result.asParameter() = p }
 
   /**
    * Gets the `Node` corresponding to a definition by reference of the variable

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

@@ -244,17 +244,49 @@ private module Cached {
     )
   }
 
+  /**
+   * Gets a `glvalue` expression whose unconverted result is the same as the
+   * unconverted result of `e`.
+   */
+  pragma[inline]
+  private Expr getAGLValueCategoryExpr(Expr e) {
+    result.isGLValueCategory() and
+    result.getUnconverted() = e.getUnconverted()
+  }
+
   /** Holds if `node` should be an `IndirectOperand` that maps `node.asExpr()` to `e`. */
   private predicate exprNodeShouldBeIndirectOperand(IndirectOperand node, Expr e, int n) {
-    exists(ArgumentOperand operand |
+    exists(ArgumentOperand operand | isIndirectOperandOfArgument(node, operand, e, n) |
+      // If `x` has type `Foo` in a call such `x.f()` the `VariableAddress`
+      // instruction (which is of type `glvalue<Foo>`) is the qualifier.
+      // So the `DataFlow::Node` we want to represent `x` is the indirection of
+      // the qualifier (which will then have type `Foo`).
+      // Since the node -> expression mapping is one-to-many we additionally need to
+      // check that the specific expression we map to is also a glvalue. For example,
+      // in `(*x).f()` the qualifier generates IR like:
+      // ```
+      // r1(glval<Foo *>) = VariableAddress[x] :
+      // r2(Foo *)        = Load[x]            : &:r1
+      // r3(glval<Foo>)   = CopyValue          : r2
+      // ```
+      // The qualifier is associated with both `*x` and the lvalue-to-rvalue
+      // conversion of `x`. Only `*x` is a glvalue and should map to the
+      // indirect qualifier node; the converted `x` is a pointer prvalue and
+      // should map to the direct qualifier node. So we require that the fully
+      // converted qualifier (i.e., the expression with `n = 0`) is a glvalue,
+      // and then map every expression in the conversion chain with the same
+      // unconverted expression to the indirect node.
+      operand.isGLValue() and
+      operand instanceof ThisArgumentOperand and
+      isIndirectOperandOfArgument(node, operand, getAGLValueCategoryExpr(e), 0)
+      or
       // When an argument (qualifier or positional) is a prvalue and the
       // parameter (qualifier or positional) is a (const) reference, IR
       // construction introduces a temporary `IRVariable`. The `VariableAddress`
       // instruction has the argument as its `getConvertedResultExpression`
       // result. However, the instruction actually represents the _address_ of
       // the argument. So to fix this mismatch, we have the indirection of the
       // `VariableAddressInstruction` map to the expression.
-      isIndirectOperandOfArgument(node, operand, e, n) and
       irTempOperandConversionFlows(operand)
     )
   }

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

@@ -39,7 +39,7 @@ void test_aggregate_literal() {
   const int ys[] = {[0] = 4, [1] = 5, [0] = 6}; // $ asExpr=4 asExpr=5 asExpr=6 asExpr={...}
 }
 
-void test_postfix_crement(int *p, int q) {
+void test_postfix_crement(int *p, int q) { // $ asParameter=p asParameter=q parameterNode=p parameterNode=q
   p++; // $ asExpr="... ++" asIndirectExpr="... ++" asExpr=p asIndirectExpr=p
   q++; // $ asExpr="... ++" asExpr=q
   (p++); // $ asExpr="... ++" asIndirectExpr="... ++" asExpr="p(... ++)" asIndirectExpr="p(*... ++)"
@@ -55,3 +55,35 @@ void test_postfix_crement(int *p, int q) {
   int *p2 = (int*)(p++); // $ asExpr="... ++" asIndirectExpr="... ++" asExpr="p(... ++)" asIndirectExpr="p(*... ++)"
   int q2 = (int)(q++); // $ asExpr="... ++" asExpr="q(... ++)"
 }
+
+struct Foo {
+  void f();
+};
+
+struct Derived : Foo {};
+
+void test_qualifiers(
+  Foo x, // $ asParameter=x parameterNode=x
+  Foo &r, // $ asParameter=r(*r) parameterNode=r(*r)
+  Foo *p, // $ asParameter=p parameterNode=p
+  Foo *&rp, // $ asParameter=rp(*rp) parameterNode=rp(*rp)
+  Derived d, // $ asParameter=d parameterNode=d
+  Derived &rd) { // $ asParameter=rd(*rd) parameterNode=rd(*rd)
+  x.f(); // $ asExpr=x asIndirectExpr=x
+  r.f(); // $ asExpr=r asIndirectExpr=r
+  p->f(); // $ asExpr=p asIndirectExpr=p
+  (*p).f(); // $ asExpr="* ..." asIndirectExpr="* ..." asExpr="p(* ...)" asIndirectExpr="p(* ...)"
+  rp->f(); // $ asExpr=rp asIndirectExpr=rp numberOfIndirectNodes="rp: 2" numberOfNodes="rp: 2"
+  (*rp).f(); // $ asExpr="* ..." asIndirectExpr="* ..." asExpr="rp(* ...)" asIndirectExpr="rp(* ...)" asIndirectExpr=rp numberOfIndirectNodes="rp: 2"
+  d.f(); // $ asExpr=d asIndirectExpr=d
+  rd.f(); // $ asExpr=rd asIndirectExpr=rd
+}
+
+void test_inner_glvalue_qualifiers(
+  Foo x, // $ asParameter=x parameterNode=x
+  Foo &r) { // $ asParameter=r(*r) parameterNode=r(*r)
+  Foo arr[2];
+  (*&x).f(); // $ asExpr="* ..." asIndirectExpr="* ..." asExpr="& ...(* ...)" asIndirectExpr="& ...(* ...)" asExpr="x(* ...)" asIndirectExpr="x(* ...)"
+  (*&r).f(); // $ asExpr="* ..." asIndirectExpr="* ..." asExpr="& ...(* ...)" asIndirectExpr="& ...(* ...)" asExpr="r(* ...)" asIndirectExpr="r(* ...)"
+  (*arr).f(); // $ asExpr="* ..." asIndirectExpr="* ..." asExpr="arr(* ...)" asIndirectExpr="arr(* ...)"
+}

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

@@ -9,8 +9,12 @@ string formatNumberOfNodesForExpr(Expr e) {
   exists(int n | n = strictcount(Node node | node.asExpr() = e) | n > 1 and result = ": " + n)
 }
 
+string formatNumberOfNodesForParameter(Parameter p) {
+  exists(int n | n = strictcount(Node node | node.asParameter() = p) | n > 1 and result = ": " + n)
+}
+
 module AsExprTest implements TestSig {
-  string getARelevantTag() { result = ["asExpr", "numberOfNodes"] }
+  string getARelevantTag() { result = ["asExpr", "numberOfNodes", "asParameter", "parameterNode"] }
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
     exists(Node n, Expr e, string exprString |
@@ -20,16 +24,42 @@ module AsExprTest implements TestSig {
       exprString = e.toString()
     |
       tag = "asExpr" and
+      // If the `toString` on the node is identical to the `toString` of the
+      // expression then we don't show it in the expected output.
+      if exprString = element
+      then value = quote(exprString)
+      else value = quote(exprString + "(" + element + ")")
+      or
+      tag = "numberOfNodes" and
+      value = quote(exprString + formatNumberOfNodesForExpr(e))
+    )
+    or
+    exists(Node n, Parameter p, string paramString |
+      not p.getName().matches("(unnamed%") and
+      location = p.getLocation() and
+      element = n.toString() and
+      paramString = p.toString()
+    |
+      p = n.asParameter() and
       (
+        tag = "asParameter" and
         // If the `toString` on the node is identical to the `toString` of the
-        // expression then we don't show it in the expected output.
-        if exprString = element
-        then value = quote(exprString)
-        else value = quote(exprString + "(" + element + ")")
+        // parameter then we don't show it in the expected output.
+        if paramString = element
+        then value = quote(paramString)
+        else value = quote(paramString + "(" + element + ")")
+        or
+        tag = "numberOfNodes" and
+        value = quote(paramString + formatNumberOfNodesForParameter(p))
       )
       or
-      tag = "numberOfNodes" and
-      value = quote(exprString + formatNumberOfNodesForExpr(e))
+      parameterNode(p) = n and
+      tag = "parameterNode" and
+      // If the `toString` on the node is identical to the `toString` of the
+      // parameter then we don't show it in the expected output.
+      if paramString = element
+      then value = quote(paramString)
+      else value = quote(paramString + "(" + element + ")")
     )
   }
 }