[clang] [clang][CFG] Fix compound assignment evaluation order (PR #212115)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Balázs Benics (steakhal)

<details>
<summary>Changes</summary>

CompoundAssignOperator (e.g. `+=`, `-=`, `<<=`) is a distinct StmtClass deriving from BinaryOperator, but it was not handled in the CFGBuilder dispatch switches. As a result it fell through to the generic default path (VisitChildren over reverse_children), which emits the LHS before the RHS in the CFG.

Per C++17 [expr.ass]/1, all assignment operators - simple and compound - sequence the right operand before the left operand. Route CompoundAssignOperator through VisitBinaryOperator (and VisitBinaryOperatorForTemporaries), whose existing isAssignmentOp() branch already emits the RHS before the LHS, matching simple assignment.

This mirrors the class of evaluation-order defect fixed for lambda captures in f3b31871e9b8.

rdar://183253943

Assisted-By: claude

---
Full diff: https://github.com/llvm/llvm-project/pull/212115.diff


3 Files Affected:

- (modified) clang/lib/Analysis/CFG.cpp (+2) 
- (added) clang/test/Analysis/cfg-compound-assignment-eval-order.cpp (+65) 
- (modified) clang/test/Analysis/loopexit-cfg-output.cpp (+3-3) 


``````````diff
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index efd1da9c9a4db..5263114ebca28 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -2367,6 +2367,7 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
       return VisitConditionalOperator(cast<BinaryConditionalOperator>(S), asc);
 
     case Stmt::BinaryOperatorClass:
+    case Stmt::CompoundAssignOperatorClass:
       return VisitBinaryOperator(cast<BinaryOperator>(S), asc);
 
     case Stmt::BlockExprClass:
@@ -5151,6 +5152,7 @@ CFGBlock *CFGBuilder::VisitForTemporaries(Stmt *E, bool ExternallyDestructed,
       return VisitChildrenForTemporaries(E, ExternallyDestructed, Context);
 
     case Stmt::BinaryOperatorClass:
+    case Stmt::CompoundAssignOperatorClass:
       return VisitBinaryOperatorForTemporaries(cast<BinaryOperator>(E),
                                                ExternallyDestructed, Context);
 
diff --git a/clang/test/Analysis/cfg-compound-assignment-eval-order.cpp b/clang/test/Analysis/cfg-compound-assignment-eval-order.cpp
new file mode 100644
index 0000000000000..f25b2daee1622
--- /dev/null
+++ b/clang/test/Analysis/cfg-compound-assignment-eval-order.cpp
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++17 %s 2>&1 | FileCheck %s
+
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus.Move \
+// RUN:   -analyzer-output=text -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+int *getPtr(int);
+int getVal(int);
+
+// Compound assignment operators sequence the RHS before the LHS, exactly like
+// simple assignment ([expr.ass]/1). The RHS (getVal) must therefore appear
+// before the LHS (getPtr) in the CFG.
+void test_builtin_compound(int a, int b) {
+  *getPtr(a) += getVal(b);
+}
+
+// CHECK-LABEL: void test_builtin_compound(int a, int b)
+// CHECK:          1: getVal
+// CHECK-NEXT:     2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, int (*)(int))
+// CHECK-NEXT:     3: b
+// CHECK-NEXT:     4: [B1.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:     5: [B1.2]([B1.4])
+// CHECK-NEXT:     6: getPtr
+// CHECK-NEXT:     7: [B1.6] (ImplicitCastExpr, FunctionToPointerDecay, int *(*)(int))
+// CHECK-NEXT:     8: a
+// CHECK-NEXT:     9: [B1.8] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:    10: [B1.7]([B1.9])
+// CHECK-NEXT:    11: *[B1.10]
+// CHECK-NEXT:    12: [B1.11] += [B1.5]
+
+struct A { A(); ~A(); };
+struct B { B(); ~B(); };
+
+int &getLHS(const A &);
+int getRHS(const B &);
+
+// The RHS temporary (B) is constructed before the LHS temporary (A), because
+// the RHS is fully sequenced before the LHS. Temporaries are destroyed in
+// reverse order of construction, so ~A() runs before ~B().
+void test_temp_dtor_order() {
+  getLHS(A()) += getRHS(B());
+}
+
+// CHECK-LABEL: void test_temp_dtor_order()
+// CHECK:       ~A() (Temporary object destructor)
+// CHECK-NEXT:  ~B() (Temporary object destructor)
+
+struct S {
+  int use(S &) const { return 1; }
+};
+
+int consume(S);
+
+// Because the RHS is evaluated before the LHS, the object 's' is moved from
+// (in the RHS) before it is used (in the LHS index computation).
+void test_move_then_use() {
+  S s;
+  int arr[10] = {};
+  arr[s.use(s)] += consume(std::move(s));
+  // expected-warning at -1 {{Method called on moved-from object 's'}}
+  // expected-note at -2    {{Method called on moved-from object 's'}}
+  // expected-note at -3    {{Object 's' is moved}}
+  (void)arr;
+}
diff --git a/clang/test/Analysis/loopexit-cfg-output.cpp b/clang/test/Analysis/loopexit-cfg-output.cpp
index 26ef33e4a6a9a..81ed2533da6c2 100644
--- a/clang/test/Analysis/loopexit-cfg-output.cpp
+++ b/clang/test/Analysis/loopexit-cfg-output.cpp
@@ -208,9 +208,9 @@ void check_dowhile1() {
 // CHECK-NEXT:   Succs (2): B4 B1
 
 // CHECK:       [B3]
-// CHECK-NEXT:   1: j
-// CHECK-NEXT:   2: 2
-// CHECK-NEXT:   3: [B3.1] += [B3.2]
+// CHECK-NEXT:   1: 2
+// CHECK-NEXT:   2: j
+// CHECK-NEXT:   3: [B3.2] += [B3.1]
 // CHECK-NEXT:   Preds (2): B4 B5
 // CHECK-NEXT:   Succs (1): B2
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/212115