[clang] [dataflow] CXXOperatorCallExpr equal operator might not be a glvalue (PR #80991)

[Paul Semel via cfe-commits]

https://github.com/paulsemel updated https://github.com/llvm/llvm-project/pull/80991

>From 0d03870ef82fac51387c353bbe4e095e431d7ce8 Mon Sep 17 00:00:00 2001
From: Paul Semel <semelpaul at gmail.com>
Date: Wed, 7 Feb 2024 13:59:40 +0000
Subject: [PATCH 1/2] [dataflow] CXXOperatorCallExpr equal operator might not
 be a glvalue

Although in a normal implementation the assumption is reasonable, it
seems that some esoteric implementation are not returning a T&. This
should be handled correctly and the values be propagated.
---
 clang/lib/Analysis/FlowSensitive/Transfer.cpp | 14 +++++++-
 .../Analysis/FlowSensitive/TransferTest.cpp   | 36 +++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index bb3aec763c29ca..d73c9d25136279 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -535,7 +535,19 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
         return;
 
       copyRecord(*LocSrc, *LocDst, Env);
-      Env.setStorageLocation(*S, *LocDst);
+
+      // If the expr is a glvalue, we can reasonably assume the operator is
+      // returning T& and thus we can assign it `LocDst`.
+      if (S->isGLValue()) {
+        Env.setStorageLocation(*S, *LocDst);
+      } else if (S->getType()->isRecordType()) {
+        // If the expr is a prvalue, we cannot really assume anything regarding
+        // the new value being created. We should just propagate it to ensure a
+        // `RecordValue` exist for it.
+        if (Env.getValue(*S) == nullptr)
+          refreshRecordValue(*S, Env);
+      }
+
       return;
     }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 8bbb04024dcce6..4a4224b42b0850 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2313,6 +2313,42 @@ TEST(TransferTest, AssignmentOperatorWithInitAndInheritance) {
          ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, AssignmentOperatorReturnsVoid) {
+  // This is a crash repro.
+  std::string Code = R"(
+    struct S {
+      void operator=(S&& other);
+    };
+    void target() {
+      S s;
+      s = S();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {});
+}
+
+TEST(TransferTest, AssignmentOperatorReturnsByValue) {
+  // This is a crash repro.
+  std::string Code = R"(
+    struct S {
+      S operator=(S&& other);
+    };
+    void target() {
+      S s;
+      s = S();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   std::string Code = R"(
     struct A {

>From 4165c1be301c1a61dda55358302436e696567e83 Mon Sep 17 00:00:00 2001
From: Paul Semel <paul.semel at epita.fr>
Date: Thu, 8 Feb 2024 15:02:04 +0100
Subject: [PATCH 2/2] Update clang/lib/Analysis/FlowSensitive/Transfer.cpp

Co-authored-by: martinboehme <mboehme at google.com>
---
 clang/lib/Analysis/FlowSensitive/Transfer.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index d73c9d25136279..fd439ef4e9ef0e 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -541,9 +541,9 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       if (S->isGLValue()) {
         Env.setStorageLocation(*S, *LocDst);
       } else if (S->getType()->isRecordType()) {
-        // If the expr is a prvalue, we cannot really assume anything regarding
-        // the new value being created. We should just propagate it to ensure a
-        // `RecordValue` exist for it.
+        // Make sure that we have a `RecordValue` for this expression so that
+        // `Environment::getResultObjectLocation()` is able to return a location
+        // for it.
         if (Env.getValue(*S) == nullptr)
           refreshRecordValue(*S, Env);
       }