C++: Move Load from AssignmentOperation to its LHS

This is analogous to what was done for `CrementOperation`.
2025-12-22 03:36:30 +01:00 · 2020-01-23 20:05:28 +01:00
parent 53b1068a9f
commit 9a45c5570d
6 changed files with 43 additions and 50 deletions
--- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll
@@ -12,7 +12,6 @@ newtype TInstructionTag =
  ZeroPadStringElementIndexTag() or
  ZeroPadStringElementAddressTag() or
  ZeroPadStringStoreTag() or
-  AssignOperationLoadTag() or
  AssignOperationConvertLeftTag() or
  AssignOperationOpTag() or
  AssignOperationConvertResultTag() or
@@ -93,8 +92,6 @@ string getInstructionTagId(TInstructionTag tag) {
  or
  tag = ZeroPadStringStoreTag() and result = "ZeroPadStore"
  or
-  tag = AssignOperationLoadTag() and result = "AssignOpLoad"
-  or
  tag = AssignOperationConvertLeftTag() and result = "AssignOpConvLeft"
  or
  tag = AssignOperationOpTag() and result = "AssignOpOp"
--- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll
@@ -229,6 +229,8 @@ private predicate ignoreLoad(Expr expr) {
 */
 private predicate needsLoadForParentExpr(Expr expr) {
  exists(CrementOperation crement | expr = crement.getOperand().getFullyConverted())
+  or
+  exists(AssignOperation ao | expr = ao.getLValue().getFullyConverted())
 }

 /**
--- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll
@@ -1299,7 +1299,7 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
  override AssignOperation expr;

  final override TranslatedElement getChild(int id) {
-    id = 0 and result = getLeftOperand()
+    id = 0 and result = getLoadedLeftOperand()
    or
    id = 1 and result = getRightOperand()
  }
@@ -1320,13 +1320,23 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
    else
      // This is C++, where the result is an lvalue for the left operand,
      // and that lvalue is not being loaded as part of this expression.
-      result = getLeftOperand().getResult()
+      result = getUnloadedLeftOperand().getResult()
  }

-  final TranslatedExpr getLeftOperand() {
+  final TranslatedExpr getUnloadedLeftOperand() { result = getLoadedLeftOperand().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 getLoadedLeftOperand() {
    result = getTranslatedExpr(expr.getLValue().getFullyConverted())
  }

+  /**
+   * Gets the address to which the result of this operation will be stored.
+   */
  final TranslatedExpr getRightOperand() {
    result = getTranslatedExpr(expr.getRValue().getFullyConverted())
  }
@@ -1334,13 +1344,6 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
    kind instanceof GotoEdge and
    (
-      (
-        tag = AssignOperationLoadTag() and
-        if leftOperandNeedsConversion()
-        then result = getInstruction(AssignOperationConvertLeftTag())
-        else result = getInstruction(AssignOperationOpTag())
-      )
-      or
      tag = AssignOperationConvertLeftTag() and
      result = getInstruction(AssignOperationOpTag())
      or
@@ -1362,10 +1365,12 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
  override Instruction getChildSuccessor(TranslatedElement child) {
    // Operands are evaluated right-to-left.
    child = getRightOperand() and
-    result = getLeftOperand().getFirstInstruction()
+    result = getLoadedLeftOperand().getFirstInstruction()
    or
-    child = getLeftOperand() and
-    result = getInstruction(AssignOperationLoadTag())
+    child = getLoadedLeftOperand() and
+    if leftOperandNeedsConversion()
+    then result = getInstruction(AssignOperationConvertLeftTag())
+    else result = getInstruction(AssignOperationOpTag())
  }

  private Instruction getStoredValue() {
@@ -1388,14 +1393,14 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
      // anyway. If we really want to model this case perfectly, we'll need the
      // extractor to tell us what the promoted type of the left operand would
      // be.
-      result = getLeftOperand().getExpr().getType()
+      result = getLoadedLeftOperand().getExpr().getType()
    else
      // The right operand has already been converted to the type of the op.
      result = getRightOperand().getExpr().getType()
  }

  private predicate leftOperandNeedsConversion() {
-    getConvertedLeftOperandType().getUnspecifiedType() != getLeftOperand()
+    getConvertedLeftOperandType().getUnspecifiedType() != getLoadedLeftOperand()
          .getExpr()
          .getUnspecifiedType()
  }
@@ -1427,10 +1432,6 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
  }

  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
-    tag = AssignOperationLoadTag() and
-    opcode instanceof Opcode::Load and
-    resultType = getTypeForPRValue(getLeftOperand().getExpr().getType())
-    or
    tag = AssignOperationOpTag() and
    opcode = getOpcode() and
    resultType = getTypeForPRValue(getConvertedLeftOperandType())
@@ -1446,7 +1447,7 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
      resultType = getTypeForPRValue(getConvertedLeftOperandType())
      or
      tag = AssignOperationConvertResultTag() and
-      resultType = getTypeForPRValue(getLeftOperand().getExpr().getType())
+      resultType = getTypeForPRValue(getLoadedLeftOperand().getExpr().getType())
    )
  }

@@ -1460,19 +1461,10 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
  }

  override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
-    tag = AssignOperationLoadTag() and
-    (
-      operandTag instanceof AddressOperandTag and
-      result = getLeftOperand().getResult()
-      or
-      operandTag instanceof LoadOperandTag and
-      result = getEnclosingFunction().getUnmodeledDefinitionInstruction()
-    )
-    or
    leftOperandNeedsConversion() and
    tag = AssignOperationConvertLeftTag() and
    operandTag instanceof UnaryOperandTag and
-    result = getInstruction(AssignOperationLoadTag())
+    result = getLoadedLeftOperand().getResult()
    or
    tag = AssignOperationOpTag() and
    (
@@ -1480,7 +1472,7 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
        operandTag instanceof LeftOperandTag and
        if leftOperandNeedsConversion()
        then result = getInstruction(AssignOperationConvertLeftTag())
-        else result = getInstruction(AssignOperationLoadTag())
+        else result = getLoadedLeftOperand().getResult()
      )
      or
      operandTag instanceof RightOperandTag and
@@ -1495,7 +1487,7 @@ class TranslatedAssignOperation extends TranslatedNonConstantExpr {
    tag = AssignmentStoreTag() and
    (
      operandTag instanceof AddressOperandTag and
-      result = getLeftOperand().getResult()
+      result = getUnloadedLeftOperand().getResult()
      or
      operandTag instanceof StoreValueOperandTag and
      result = getStoredValue()
--- a/cpp/ql/test/library-tests/dataflow/crement/crements.cpp
+++ b/cpp/ql/test/library-tests/dataflow/crement/crements.cpp
@@ -6,10 +6,10 @@ void test_crement() {
  x2++;

  int x3 = 0;
-  x3 -= 1; // flow [NOT DETECTED]
+  x3 -= 1; // flow

  int x4 = 0;
-  x4 |= 1; // flow [NOT DETECTED]
+  x4 |= 1; // flow

  int x5 = 0;
  x5 = x5 - 1; // flow (to RHS)
--- a/cpp/ql/test/library-tests/dataflow/crement/from0.expected
+++ b/cpp/ql/test/library-tests/dataflow/crement/from0.expected
@@ -1,3 +1,5 @@
 | crements.cpp:3:5:3:6 | x1 |
 | crements.cpp:6:3:6:4 | x2 |
+| crements.cpp:9:3:9:4 | x3 |
+| crements.cpp:12:3:12:4 | x4 |
 | crements.cpp:15:8:15:9 | x5 |
--- a/cpp/ql/test/library-tests/rangeanalysis/signanalysis/SignAnalysis.expected
+++ b/cpp/ql/test/library-tests/rangeanalysis/signanalysis/SignAnalysis.expected
@@ -71,8 +71,8 @@
 | test.c:33:26:33:26 | Load: i | positive |
 | test.c:33:26:33:28 | Add: ... + ... | positive strictlyPositive |
 | test.c:33:28:33:28 | Constant: 1 | positive strictlyPositive |
+| test.c:34:5:34:9 | Load: total | positive |
 | test.c:34:5:34:14 | Add: ... += ... | positive |
-| test.c:34:5:34:14 | Load: ... += ... | positive |
 | test.c:34:5:34:14 | Store: ... += ... | positive |
 | test.c:34:14:34:14 | Load: i | positive |
 | test.c:36:10:36:14 | Load: total | positive |
@@ -87,8 +87,8 @@
 | test.c:42:22:42:24 | Add: ... ++ | positive strictlyPositive |
 | test.c:42:22:42:24 | Constant: ... ++ | positive strictlyPositive |
 | test.c:42:22:42:24 | Store: ... ++ | positive strictlyPositive |
+| test.c:43:5:43:9 | Load: total | positive |
 | test.c:43:5:43:14 | Add: ... += ... | positive |
-| test.c:43:5:43:14 | Load: ... += ... | positive |
 | test.c:43:5:43:14 | Store: ... += ... | positive |
 | test.c:43:14:43:14 | Load: i | positive |
 | test.c:45:10:45:14 | Load: total | positive |
@@ -105,8 +105,8 @@
 | test.c:51:28:51:28 | Load: i | positive |
 | test.c:51:28:51:30 | Add: ... + ... | positive strictlyPositive |
 | test.c:51:30:51:30 | Constant: 1 | positive strictlyPositive |
+| test.c:52:5:52:9 | Load: total | positive |
 | test.c:52:5:52:14 | Add: ... += ... | positive |
-| test.c:52:5:52:14 | Load: ... += ... | positive |
 | test.c:52:5:52:14 | Store: ... += ... | positive |
 | test.c:52:14:52:14 | Load: i | positive |
 | test.c:54:10:54:14 | Load: total | positive |
@@ -148,8 +148,8 @@
 | test.c:124:36:124:36 | Constant: (unsigned long long)... | positive strictlyPositive |
 | test.c:126:31:126:43 | Call: call to test12_helper | positive |
 | test.c:126:31:126:43 | Store: call to test12_helper | positive |
+| test.c:127:6:127:10 | Load: Start | positive |
 | test.c:127:6:127:24 | Add: ... += ... | positive strictlyPositive |
-| test.c:127:6:127:24 | Load: ... += ... | positive |
 | test.c:127:6:127:24 | Store: ... += ... | positive strictlyPositive |
 | test.c:127:15:127:20 | Load: Length | positive |
 | test.c:127:15:127:24 | Add: ... + ... | positive strictlyPositive |
@@ -253,8 +253,8 @@
 | test.c:205:13:205:15 | Mul: ... * ... | positive |
 | test.c:205:13:205:15 | Store: ... * ... | positive |
 | test.c:205:15:205:15 | Load: b | positive |
+| test.c:206:5:206:9 | Load: total | positive |
 | test.c:206:5:206:14 | Add: ... += ... | positive |
-| test.c:206:5:206:14 | Load: ... += ... | positive |
 | test.c:206:5:206:14 | Store: ... += ... | positive |
 | test.c:206:14:206:14 | Load: r | positive |
 | test.c:208:7:208:7 | Constant: 3 | positive strictlyPositive |
@@ -264,7 +264,7 @@
 | test.c:208:28:208:30 | Constant: - ... | negative strictlyNegative |
 | test.c:208:45:208:46 | Constant: 23 | positive strictlyPositive |
 | test.c:209:13:209:13 | Load: a | positive strictlyPositive |
-| test.c:210:5:210:14 | Load: ... += ... | positive |
+| test.c:210:5:210:9 | Load: total | positive |
 | test.c:212:7:212:7 | Constant: 3 | positive strictlyPositive |
 | test.c:212:17:212:17 | Load: a | positive strictlyPositive |
 | test.c:212:22:212:23 | Constant: 11 | positive strictlyPositive |
@@ -305,8 +305,8 @@
 | test.c:233:13:233:15 | Mul: ... * ... | positive |
 | test.c:233:13:233:15 | Store: ... * ... | positive |
 | test.c:233:15:233:15 | Load: b | positive |
+| test.c:234:5:234:9 | Load: total | positive |
 | test.c:234:5:234:14 | Add: ... += ... | positive |
-| test.c:234:5:234:14 | Load: ... += ... | positive |
 | test.c:234:5:234:14 | Store: ... += ... | positive |
 | test.c:234:14:234:14 | Load: r | positive |
 | test.c:236:7:236:7 | Phi: 0 | positive |
@@ -315,7 +315,7 @@
 | test.c:236:28:236:30 | Constant: - ... | negative strictlyNegative |
 | test.c:236:45:236:46 | Constant: 23 | positive strictlyPositive |
 | test.c:237:13:237:13 | Load: a | positive |
-| test.c:238:5:238:14 | Load: ... += ... | positive |
+| test.c:238:5:238:9 | Load: total | positive |
 | test.c:240:17:240:17 | Load: a | positive |
 | test.c:240:22:240:23 | Constant: 11 | positive strictlyPositive |
 | test.c:240:28:240:30 | Constant: - ... | negative strictlyNegative |
@@ -376,8 +376,8 @@
 | test.c:289:13:289:15 | Mul: ... * ... | negative |
 | test.c:289:13:289:15 | Store: ... * ... | negative |
 | test.c:289:15:289:15 | Load: b | positive |
+| test.c:290:5:290:9 | Load: total | negative |
 | test.c:290:5:290:14 | Add: ... += ... | negative |
-| test.c:290:5:290:14 | Load: ... += ... | negative |
 | test.c:290:5:290:14 | Store: ... += ... | negative |
 | test.c:290:14:290:14 | Load: r | negative |
 | test.c:292:7:292:9 | Constant: - ... | negative strictlyNegative |
@@ -385,7 +385,7 @@
 | test.c:292:29:292:31 | Constant: - ... | negative strictlyNegative |
 | test.c:292:46:292:47 | Constant: 23 | positive strictlyPositive |
 | test.c:293:13:293:13 | Load: a | negative |
-| test.c:294:5:294:14 | Load: ... += ... | negative |
+| test.c:294:5:294:9 | Load: total | negative |
 | test.c:296:7:296:9 | Constant: - ... | negative strictlyNegative |
 | test.c:296:29:296:31 | Constant: - ... | negative strictlyNegative |
 | test.c:297:13:297:13 | Load: a | negative |
@@ -422,8 +422,8 @@
 | test.c:317:13:317:15 | Mul: ... * ... | negative |
 | test.c:317:13:317:15 | Store: ... * ... | negative |
 | test.c:317:15:317:15 | Load: b | positive |
+| test.c:318:5:318:9 | Load: total | negative |
 | test.c:318:5:318:14 | Add: ... += ... | negative |
-| test.c:318:5:318:14 | Load: ... += ... | negative |
 | test.c:318:5:318:14 | Store: ... += ... | negative |
 | test.c:318:14:318:14 | Load: r | negative |
 | test.c:320:7:320:9 | Constant: - ... | negative strictlyNegative |
@@ -432,7 +432,7 @@
 | test.c:320:30:320:32 | Constant: - ... | negative strictlyNegative |
 | test.c:320:47:320:48 | Constant: 23 | positive strictlyPositive |
 | test.c:321:13:321:13 | Load: a | negative strictlyNegative |
-| test.c:322:5:322:14 | Load: ... += ... | negative |
+| test.c:322:5:322:9 | Load: total | negative |
 | test.c:324:7:324:9 | Constant: - ... | negative strictlyNegative |
 | test.c:324:24:324:25 | Constant: - ... | negative strictlyNegative |
 | test.c:324:30:324:32 | Constant: - ... | negative strictlyNegative |
@@ -657,8 +657,8 @@
 | test.c:398:9:398:11 | Constant: ... ++ | positive strictlyPositive |
 | test.c:398:9:398:11 | Store: ... ++ | positive strictlyPositive |
 | test.c:398:9:398:22 | CopyValue: ... , ... | positive strictlyPositive |
+| test.c:398:14:398:14 | Load: y | positive strictlyPositive |
 | test.c:398:14:398:19 | Add: ... += ... | positive strictlyPositive |
-| test.c:398:14:398:19 | Load: ... += ... | positive strictlyPositive |
 | test.c:398:14:398:19 | Store: ... += ... | positive strictlyPositive |
 | test.c:398:19:398:19 | Constant: (unsigned int)... | positive strictlyPositive |
 | test.c:398:22:398:22 | Load: y | positive strictlyPositive |