https://github.com/steakhal created https://github.com/llvm/llvm-project/pull/212115
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 From 021ee9dba1f8369ddb858a92c2026ccef3e2a5b1 Mon Sep 17 00:00:00 2001 From: Balazs Benics <[email protected]> Date: Sun, 26 Jul 2026 13:03:32 +0100 Subject: [PATCH] [clang][CFG] Fix compound assignment evaluation order 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 --- clang/lib/Analysis/CFG.cpp | 2 + .../cfg-compound-assignment-eval-order.cpp | 65 +++++++++++++++++++ clang/test/Analysis/loopexit-cfg-output.cpp | 6 +- 3 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 clang/test/Analysis/cfg-compound-assignment-eval-order.cpp 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@-1 {{Method called on moved-from object 's'}} + // expected-note@-2 {{Method called on moved-from object 's'}} + // expected-note@-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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
