[dataflow] CXXOperatorCallExpr equal operator might not be a glvalue

[paulsemel]

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.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Paul Semel (paulsemel)

Changes

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.

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+13-1)
• (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+36)

diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index bb3aec763c29ca..fb1c6848197339 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() && S->isPRValue()) {
+        // 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..86f4081c798d55 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, CXXOperatorCallExprEqualReturnsVoid) {
+  // This is a crash repro.
+  std::string Code = R"(
+    struct B {
+      void operator=(B&& other);
+    };
+    void target() {
+      B b;
+      b = B();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {});
+}
+
+TEST(TransferTest, CXXOperatorCallExprEqualReturnsPRValue) {
+  // This is a crash repro.
+  std::string Code = R"(
+    struct B {
+      B operator=(B&& other);
+    };
+    void target() {
+      B b;
+      b = B();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {});
+}
+
 TEST(TransferTest, CopyConstructor) {
   std::string Code = R"(
     struct A {

[martinboehme]

Is this actually necessary? (It's not really doing anything?)

[paulsemel]

Yes, similarly to TransferCallExpr, it ensures a RecordValue exists. This is just a short path for not having to call TransferCallExpr, since it is pretty much the only thing that will trigger if being called.

[martinboehme]

Got it. Added a suggested edit for the comment.

[martinboehme]

Got it. Added a suggested edit for the comment.