[clang] 6489648 - [clang][dataflow] Add basic modeling for compound assignments. (#179058)

[via cfe-commits]

Author: Artem Dergachev
Date: 2026-03-11T09:50:41-04:00
New Revision: 6489648f079019389fd44c76c5b760c547ffca09

URL: https://github.com/llvm/llvm-project/commit/6489648f079019389fd44c76c5b760c547ffca09
DIFF: https://github.com/llvm/llvm-project/commit/6489648f079019389fd44c76c5b760c547ffca09.diff

LOG: [clang][dataflow] Add basic modeling for compound assignments. (#179058)

Do our best to assign a value to the left-hand-side storage. Do not
model the actual arithmetic operation yet; the value is going to be
unknown in case of compound assignments. But either way, simularly to
#178943,we need to at least make sure that the old value does not stick
around. And it's better to conjure a fresh value than to leave it
completely unmodeled because subsequent loads from that location need to
produce consistent results.

Additionally make sure that the storage location is correctly propagated
in regular assignments too, even if we couldn't produce a fresh value at
all.

---------

Co-authored-by: Yitzhak Mandelbaum <ymand at users.noreply.github.com>

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Transfer.cpp
    clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 445f39b43f683..57ee93f5e156e 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -155,38 +155,40 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     const Expr *RHS = S->getRHS();
     assert(RHS != nullptr);
 
-    // Do compound assignments up-front, as there are so many of them and we
-    // don't want to list all of them in the switch statement below.
-    // To avoid generating unnecessary values, we don't create a new value but
-    // instead leave it to the specific analysis to do this if desired.
-    if (S->isCompoundAssignmentOp())
-      propagateStorageLocation(*S->getLHS(), *S, Env);
-
-    switch (S->getOpcode()) {
-    case BO_Assign: {
+    // Do assignments and compound assignments up-front, as there are
+    // so many of them and we don't want to list all of them in
+    // the switch statement below.
+    if (S->isAssignmentOp()) {
       auto *LHSLoc = Env.getStorageLocation(*LHS);
       if (LHSLoc == nullptr)
-        break;
+        return;
 
-      auto *RHSVal = Env.getValue(*RHS);
+      // Assign a storage location for the whole expression.
+      Env.setStorageLocation(*S, *LHSLoc);
+
+      // Compound assignments involve arithmetic we don't model yet.
+      // Regular assignments preserve the value so they're easy.
+      Value *RHSVal =
+          S->isCompoundAssignmentOp() ? nullptr : Env.getValue(*RHS);
       if (RHSVal == nullptr) {
+        // Either way, we need to conjure a value if we don't have any so that
+        // future lookups into that location produce consistent results.
         RHSVal = Env.createValue(LHS->getType());
         if (RHSVal == nullptr) {
           // At least make sure the old value is gone. It's unlikely to be there
           // in the first place given that we don't even know how to create
           // a basic unknown value of that type.
           Env.clearValue(*LHSLoc);
-          break;
+          return;
         }
       }
 
       // Assign a value to the storage location of the left-hand side.
       Env.setValue(*LHSLoc, *RHSVal);
-
-      // Assign a storage location for the whole expression.
-      Env.setStorageLocation(*S, *LHSLoc);
-      break;
+      return;
     }
+
+    switch (S->getOpcode()) {
     case BO_LAnd:
     case BO_LOr: {
       BoolValue &LHSVal = getLogicOperatorSubExprValue(*LHS);

diff  --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 3c79c367415aa..e147f3dc7a195 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -990,8 +990,7 @@ TEST(TransferTest, BinaryOperatorAssignUnknown) {
         ASSERT_TRUE(isa_and_nonnull<IntegerValue>(FooAtCVal));
 
         EXPECT_NE(FooAtAVal, FooAtBVal);
-        // FIXME: Should be NE too.
-        EXPECT_EQ(FooAtBVal, FooAtCVal);
+        EXPECT_NE(FooAtBVal, FooAtCVal);
 
         // Check that the storage location is correctly propagated.
         auto MatchResult = match(binaryOperator().bind("bo"), ASTCtx);
@@ -1006,8 +1005,7 @@ TEST(TransferTest, BinaryOperatorAssignUnknown) {
         const Environment &EnvR = getEnvironmentAtAnnotation(Results, "r");
 
         EXPECT_FALSE(EnvQ.proves(EnvQ.arena().makeLiteral(false)));
-        // FIXME: Should be FALSE too.
-        EXPECT_TRUE(EnvR.proves(EnvR.arena().makeLiteral(false)));
+        EXPECT_FALSE(EnvR.proves(EnvR.arena().makeLiteral(false)));
       });
 }
 
@@ -1048,14 +1046,13 @@ TEST(TransferTest, BinaryOperatorAssignFloat) {
         // FIXME: Should be non-null. Floats aren't modeled at all.
         EXPECT_THAT(FooAtBVal, IsNull());
 
-        // See if the storage location is correctly propagated.
+        // Check that the storage location is correctly propagated.
         auto MatchResult =
             match(binaryOperator(hasOperatorName("=")).bind("bo"), ASTCtx);
         const auto *BO = selectFirst<BinaryOperator>("bo", MatchResult);
         ASSERT_THAT(BO, NotNull());
         const StorageLocation *BOLoc = EnvP.getStorageLocation(*BO);
-        // FIXME: Should be non-null.
-        EXPECT_THAT(BOLoc, IsNull());
+        EXPECT_THAT(BOLoc, NotNull());
       });
 }