C++: IR: Result of x in x++ is now the Load

Previously, the `Load` would be associated with the `CrementOperation`
rather than its operand, which gave surprising results when mapping
taint sinks back to `Expr`.

The changes in `raw_ir.expected` are to add `Copy` operations on the
`x++` in code like `y = x++`. This is now needed because the result that
`x++` would otherwise have (the Load) no longer belongs to the `++`
expression. Copies are inserted to ensure that all expressions are
associated with an `Instruction` result.

The changes in `*aliased_ssa_ir.expected` appear to be just wobble.

Author: jbj

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll

@@ -17,7 +17,6 @@ newtype TInstructionTag =
   AssignOperationOpTag() or
   AssignOperationConvertResultTag() or
   AssignmentStoreTag() or
-  CrementLoadTag() or
   CrementConstantTag() or
   CrementOpTag() or
   CrementStoreTag() or
@@ -104,8 +103,6 @@ string getInstructionTagId(TInstructionTag tag) {
   or
   tag = AssignmentStoreTag() and result = "AssignStore"
   or
-  tag = CrementLoadTag() and result = "CrementLoad"
-  or
   tag = CrementConstantTag() and result = "CrementConst"
   or
   tag = CrementOpTag() and result = "CrementOp"

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -208,7 +208,7 @@ private predicate usedAsCondition(Expr expr) {
  * AST as an lvalue-to-rvalue conversion, but the IR represents both a function
  * lvalue and a function pointer prvalue the same.
  */
-predicate ignoreLoad(Expr expr) {
+private predicate ignoreLoad(Expr expr) {
   expr.hasLValueToRValueConversion() and
   (
     expr instanceof ThisExpr or
@@ -220,6 +220,32 @@ predicate ignoreLoad(Expr expr) {
   )
 }
 
+/**
+ * Holds if `expr` should have a load on it because it will be loaded as part
+ * of the translation of its parent. We want to associate this load with `expr`
+ * itself rather than its parent since in practical applications like data flow
+ * we maintain that the value of the `x` in `x++` should be what's loaded from
+ * `x`.
+ */
+private predicate needsLoadForParentExpr(Expr expr) {
+  exists(CrementOperation crement | expr = crement.getOperand().getFullyConverted())
+}
+
+/**
+ * Holds if `expr` should have a `TranslatedLoad` on it.
+ */
+predicate hasTranslatedLoad(Expr expr) {
+  (
+    expr.hasLValueToRValueConversion()
+    or
+    needsLoadForParentExpr(expr)
+  ) and
+  not ignoreExpr(expr) and
+  not isNativeCondition(expr) and
+  not isFlexibleCondition(expr) and
+  not ignoreLoad(expr)
+}
+
 newtype TTranslatedElement =
   // An expression that is not being consumed as a condition
   TTranslatedValueExpr(Expr expr) {
@@ -229,21 +255,12 @@ newtype TTranslatedElement =
   } or
   // A separate element to handle the lvalue-to-rvalue conversion step of an
   // expression.
-  TTranslatedLoad(Expr expr) {
-    not ignoreExpr(expr) and
-    not isNativeCondition(expr) and
-    not isFlexibleCondition(expr) and
-    expr.hasLValueToRValueConversion() and
-    not ignoreLoad(expr)
-  } or
+  TTranslatedLoad(Expr expr) { hasTranslatedLoad(expr) } or
+  // For expressions that would not otherwise generate an instruction.
   TTranslatedResultCopy(Expr expr) {
     not ignoreExpr(expr) and
     exprNeedsCopyIfNotLoaded(expr) and
-    // Doesn't have a TTranslatedLoad
-    not (
-      expr.hasLValueToRValueConversion() and
-      not ignoreLoad(expr)
-    )
+    not hasTranslatedLoad(expr)
   } or
   // An expression most naturally translated as control flow.
   TTranslatedNativeCondition(Expr expr) {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -63,9 +63,12 @@ abstract class TranslatedExpr extends TranslatedElement {
    * Holds if the result of this `TranslatedExpr` is a glvalue.
    */
   predicate isResultGLValue() {
-    // This implementation is overridden in `TranslatedCoreExpr` to mark them
-    // as glvalues if they have loads on them. It's not overridden in
-    // `TranslatedResultCopy` since result copies never have loads.
+    // This implementation is overridden in `TranslatedCoreExpr` to mark them as
+    // glvalues if they have loads on them. It's also overridden in
+    // `TranslatedLoad` to always mark loads as glvalues since a
+    // `TranslatedLoad` may have been created as a result of
+    // `needsLoadForParentExpr`. It's not overridden in `TranslatedResultCopy`
+    // since result copies never have loads.
     expr.isGLValueCategory()
   }
 
@@ -103,18 +106,13 @@ abstract class TranslatedCoreExpr extends TranslatedExpr {
     or
     // If this TranslatedExpr doesn't produce the result, then it must represent
     // a glvalue that is then loaded by a TranslatedLoad.
-    hasLoad()
-  }
-
-  final predicate hasLoad() {
-    expr.hasLValueToRValueConversion() and
-    not ignoreLoad(expr)
+    hasTranslatedLoad(expr)
   }
 
   final override predicate producesExprResult() {
     // If there's no load, then this is the only TranslatedExpr for this
     // expression.
-    not hasLoad() and
+    not hasTranslatedLoad(expr) and
     // If there's a result copy, then this expression's result is the copy.
     not exprNeedsCopyIfNotLoaded(expr)
   }
@@ -270,6 +268,8 @@ class TranslatedLoad extends TranslatedExpr, TTranslatedLoad {
     resultType = getResultType()
   }
 
+  override predicate isResultGLValue() { none() }
+
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     tag = LoadTag() and
     result = getParent().getChildSuccessor(this) and
@@ -298,7 +298,7 @@ class TranslatedLoad extends TranslatedExpr, TTranslatedLoad {
     any()
   }
 
-  private TranslatedCoreExpr getOperand() { result.getExpr() = expr }
+  TranslatedCoreExpr getOperand() { result.getExpr() = expr }
 }
 
 /**
@@ -387,7 +387,7 @@ private int getElementSize(Type type) {
 abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   override CrementOperation expr;
 
-  final override TranslatedElement getChild(int id) { id = 0 and result = getOperand() }
+  final override TranslatedElement getChild(int id) { id = 0 and result = getLoadedOperand() }
 
   final override string getInstructionConstantValue(InstructionTag tag) {
     tag = CrementConstantTag() and
@@ -416,10 +416,6 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
-    tag = CrementLoadTag() and
-    opcode instanceof Opcode::Load and
-    resultType = getTypeForPRValue(expr.getType())
-    or
     tag = CrementConstantTag() and
     opcode instanceof Opcode::Constant and
     resultType = getConstantType()
@@ -434,19 +430,10 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
-    tag = CrementLoadTag() and
-    (
-      operandTag instanceof AddressOperandTag and
-      result = getOperand().getResult()
-      or
-      operandTag instanceof LoadOperandTag and
-      result = getEnclosingFunction().getUnmodeledDefinitionInstruction()
-    )
-    or
     tag = CrementOpTag() and
     (
       operandTag instanceof LeftOperandTag and
-      result = getInstruction(CrementLoadTag())
+      result = getLoadedOperand().getResult()
       or
       operandTag instanceof RightOperandTag and
       result = getInstruction(CrementConstantTag())
@@ -455,21 +442,20 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
     tag = CrementStoreTag() and
     (
       operandTag instanceof AddressOperandTag and
-      result = getOperand().getResult()
+      result = getUnloadedOperand().getResult()
       or
       operandTag instanceof StoreValueOperandTag and
       result = getInstruction(CrementOpTag())
     )
   }
 
-  final override Instruction getFirstInstruction() { result = getOperand().getFirstInstruction() }
+  final override Instruction getFirstInstruction() {
+    result = getLoadedOperand().getFirstInstruction()
+  }
 
   final override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     kind instanceof GotoEdge and
     (
-      tag = CrementLoadTag() and
-      result = getInstruction(CrementConstantTag())
-      or
       tag = CrementConstantTag() and
       result = getInstruction(CrementOpTag())
       or
@@ -482,7 +468,7 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
   }
 
   final override Instruction getChildSuccessor(TranslatedElement child) {
-    child = getOperand() and result = getInstruction(CrementLoadTag())
+    child = getLoadedOperand() and result = getInstruction(CrementConstantTag())
   }
 
   final override int getInstructionElementSize(InstructionTag tag) {
@@ -494,10 +480,20 @@ abstract class TranslatedCrementOperation extends TranslatedNonConstantExpr {
     result = getElementSize(expr.getType())
   }
 
-  final TranslatedExpr getOperand() {
+  /**
+   * Gets the `TranslatedLoad` on the `e` in this `e++`, which is the element
+   * that holds the value to be cremented. It's guaranteed that there's a load
+   * on `e` because of the `needsLoadForParentExpr` predicate.
+   */
+  final TranslatedLoad getLoadedOperand() {
     result = getTranslatedExpr(expr.getOperand().getFullyConverted())
   }
 
+  /**
+   * Gets the address to which the result of this crement will be stored.
+   */
+  final TranslatedExpr getUnloadedOperand() { result = getLoadedOperand().getOperand() }
+
   final Opcode getOpcode() {
     exists(Type resultType |
       resultType = expr.getUnspecifiedType() and
@@ -534,17 +530,14 @@ class TranslatedPrefixCrementOperation extends TranslatedCrementOperation {
     else
       // This is C++, where the result is an lvalue for the operand, and that
       // lvalue is not being loaded as part of this expression.
-      result = getOperand().getResult()
+      result = getUnloadedOperand().getResult()
   }
 }
 
 class TranslatedPostfixCrementOperation extends TranslatedCrementOperation {
   override PostfixCrementOperation expr;
 
-  override Instruction getResult() {
-    // The result is a prvalue copy of the original value
-    result = getInstruction(CrementLoadTag())
-  }
+  override Instruction getResult() { result = getLoadedOperand().getResult() }
 }
 
 /**
@@ -2476,6 +2469,9 @@ predicate exprNeedsCopyIfNotLoaded(Expr expr) {
     expr instanceof PrefixCrementOperation and
     not expr.isPRValueCategory() // is C++
     or
+    // Because the load is on the `e` in `e++`.
+    expr instanceof PostfixCrementOperation
+    or
     expr instanceof PointerDereferenceExpr
     or
     expr instanceof AddressOfExpr

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

@@ -1,9 +1,9 @@
 void test_crement() {
   int x1 = 0;
-  ++x1; // flow [NOT DETECTED]
+  ++x1;
 
   int x2 = 0;
-  x2++; // flow [NOT DETECTED]
+  x2++;
 
   int x3 = 0;
   x3 -= 1; // flow [NOT DETECTED]

cpp/ql/test/library-tests/dataflow/crement/from0.expected

@@ -1 +1,3 @@
+| crements.cpp:3:5:3:6 | x1 |
+| crements.cpp:6:3:6:4 | x2 |
 | crements.cpp:15:8:15:9 | x5 |

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

@@ -509,15 +509,17 @@ ir.cpp:
 #  103|     r103_3(int)        = Constant[1]            : 
 #  103|     r103_4(int)        = Add                    : r103_2, r103_3
 #  103|     mu103_5(int)       = Store                  : &:r103_1, r103_4
-#  103|     r103_6(glval<int>) = VariableAddress[y]     : 
-#  103|     mu103_7(int)       = Store                  : &:r103_6, r103_2
+#  103|     r103_6(int)        = CopyValue              : r103_2
+#  103|     r103_7(glval<int>) = VariableAddress[y]     : 
+#  103|     mu103_8(int)       = Store                  : &:r103_7, r103_6
 #  104|     r104_1(glval<int>) = VariableAddress[x]     : 
 #  104|     r104_2(int)        = Load                   : &:r104_1, ~mu98_3
 #  104|     r104_3(int)        = Constant[1]            : 
 #  104|     r104_4(int)        = Sub                    : r104_2, r104_3
 #  104|     mu104_5(int)       = Store                  : &:r104_1, r104_4
-#  104|     r104_6(glval<int>) = VariableAddress[y]     : 
-#  104|     mu104_7(int)       = Store                  : &:r104_6, r104_2
+#  104|     r104_6(int)        = CopyValue              : r104_2
+#  104|     r104_7(glval<int>) = VariableAddress[y]     : 
+#  104|     mu104_8(int)       = Store                  : &:r104_7, r104_6
 #  105|     v105_1(void)       = NoOp                   : 
 #   98|     v98_6(void)        = ReturnVoid             : 
 #   98|     v98_7(void)        = UnmodeledUse           : mu*
@@ -727,15 +729,17 @@ ir.cpp:
 #  149|     r149_3(float)        = Constant[1.0]          : 
 #  149|     r149_4(float)        = Add                    : r149_2, r149_3
 #  149|     mu149_5(float)       = Store                  : &:r149_1, r149_4
-#  149|     r149_6(glval<float>) = VariableAddress[y]     : 
-#  149|     mu149_7(float)       = Store                  : &:r149_6, r149_2
+#  149|     r149_6(float)        = CopyValue              : r149_2
+#  149|     r149_7(glval<float>) = VariableAddress[y]     : 
+#  149|     mu149_8(float)       = Store                  : &:r149_7, r149_6
 #  150|     r150_1(glval<float>) = VariableAddress[x]     : 
 #  150|     r150_2(float)        = Load                   : &:r150_1, ~mu144_3
 #  150|     r150_3(float)        = Constant[1.0]          : 
 #  150|     r150_4(float)        = Sub                    : r150_2, r150_3
 #  150|     mu150_5(float)       = Store                  : &:r150_1, r150_4
-#  150|     r150_6(glval<float>) = VariableAddress[y]     : 
-#  150|     mu150_7(float)       = Store                  : &:r150_6, r150_2
+#  150|     r150_6(float)        = CopyValue              : r150_2
+#  150|     r150_7(glval<float>) = VariableAddress[y]     : 
+#  150|     mu150_8(float)       = Store                  : &:r150_7, r150_6
 #  151|     v151_1(void)         = NoOp                   : 
 #  144|     v144_6(void)         = ReturnVoid             : 
 #  144|     v144_7(void)         = UnmodeledUse           : mu*
@@ -1037,15 +1041,17 @@ ir.cpp:
 #  209|     r209_3(int)          = Constant[1]              : 
 #  209|     r209_4(int *)        = PointerAdd[4]            : r209_2, r209_3
 #  209|     mu209_5(int *)       = Store                    : &:r209_1, r209_4
-#  209|     r209_6(glval<int *>) = VariableAddress[q]       : 
-#  209|     mu209_7(int *)       = Store                    : &:r209_6, r209_2
+#  209|     r209_6(int *)        = CopyValue                : r209_2
+#  209|     r209_7(glval<int *>) = VariableAddress[q]       : 
+#  209|     mu209_8(int *)       = Store                    : &:r209_7, r209_6
 #  210|     r210_1(glval<int *>) = VariableAddress[p]       : 
 #  210|     r210_2(int *)        = Load                     : &:r210_1, ~mu204_3
 #  210|     r210_3(int)          = Constant[1]              : 
 #  210|     r210_4(int *)        = PointerSub[4]            : r210_2, r210_3
 #  210|     mu210_5(int *)       = Store                    : &:r210_1, r210_4
-#  210|     r210_6(glval<int *>) = VariableAddress[q]       : 
-#  210|     mu210_7(int *)       = Store                    : &:r210_6, r210_2
+#  210|     r210_6(int *)        = CopyValue                : r210_2
+#  210|     r210_7(glval<int *>) = VariableAddress[q]       : 
+#  210|     mu210_8(int *)       = Store                    : &:r210_7, r210_6
 #  211|     v211_1(void)         = NoOp                     : 
 #  204|     v204_8(void)         = ReturnIndirection        : &:r204_6, ~mu204_3
 #  204|     v204_9(void)         = ReturnVoid               :