[LifetimeSafety] Track moved declarations to prevent false positives

[usx95]

Prevent false positives in lifetime safety analysis when variables are moved using std::move.

When a value is moved using std::move, ownership is transferred from the original variable to another. The lifetime safety analysis was previously generating false positives by warning about use-after-lifetime when the original variable was destroyed after being moved. This change prevents those false positives by tracking moved declarations and exempting them from loan expiration checks.

• Added tracking for declarations that have been moved via std::move in the FactsGenerator class
• Added a MovedDecls set to track moved declarations in a flow-insensitive manner
• Implemented detection of std::move calls in VisitCallExpr
• Modified handleLifetimeEnds to skip loans for declarations that have been moved
• Added a test case demonstrating that warnings are suppressed for moved variables

void silenced() {
  MyObj b;
  View v;
  {
    MyObj a;
    v = a;
    b = std::move(a); // No warning for 'a' being moved.
  }
  (void)v;
}

Fixes #152520

[usx95]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [LifetimeSafety] Track moved declarations to prevent false positives #170007 👈 (View in Graphite)
• [LifetimeSafety] Add origin tracking for pointer dereference #170006
• [LifetimeSafety] Add implicit tracking for STL functions #170005
• [LifetimeSafety] Implement multi-level origins #168344
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

🐧 Linux x64 Test Results

• 111427 tests passed
• 4450 tests skipped

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang-temporal-safety

Author: Utkarsh Saxena (usx95)

Changes

Prevent false positives in lifetime safety analysis when variables are moved using std::move.

When a value is moved using std::move, ownership is transferred from the original variable to another. The lifetime safety analysis was previously generating false positives by warning about use-after-lifetime when the original variable was destroyed after being moved. This change prevents those false positives by tracking moved declarations and exempting them from loan expiration checks.

• Added tracking for declarations that have been moved via std::move in the FactsGenerator class
• Added a MovedDecls set to track moved declarations in a flow-insensitive manner
• Implemented detection of std::move calls in VisitCallExpr
• Modified handleLifetimeEnds to skip loans for declarations that have been moved
• Added a test case demonstrating that warnings are suppressed for moved variables

void silenced() {
  MyObj b;
  View v;
  {
    MyObj a;
    v = a;
    b = std::move(a); // No warning for 'a' being moved.
  }
  (void)v;
}

Fixes #152520

---

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

3 Files Affected:

• (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+5)
• (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+23)
• (modified) clang/test/Sema/warn-lifetime-safety.cpp (+18)

diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
index 5b5626020e772..0c0581239ce34 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
@@ -101,6 +101,11 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> {
   // corresponding to the left-hand side is updated to be a "write", thereby
   // exempting it from the check.
   llvm::DenseMap<const DeclRefExpr *, UseFact *> UseFacts;
+
+  // Tracks declarations that have been moved via std::move. This is used to
+  // prevent false positives when the original owner is destroyed after the
+  // value has been moved. This tracking is flow-insensitive.
+  llvm::DenseSet<const ValueDecl *> MovedDecls;
 };
 
 } // namespace clang::lifetimes::internal
diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
index b27dcb6163449..ba88af2418056 100644
--- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
@@ -163,9 +163,27 @@ void FactsGenerator::VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
   }
 }
 
+static bool isStdMove(const FunctionDecl *FD) {
+  return FD && FD->isInStdNamespace() && FD->getIdentifier() &&
+         FD->getName() == "move";
+}
+
 void FactsGenerator::VisitCallExpr(const CallExpr *CE) {
   handleFunctionCall(CE, CE->getDirectCallee(),
                      {CE->getArgs(), CE->getNumArgs()});
+  // Track declarations that are moved via std::move.
+  // This is a flow-insensitive approximation: once a declaration is moved
+  // anywhere in the function, it's treated as moved everywhere. This can lead
+  // to false negatives on control flow paths where the value is not actually
+  // moved, but these are considered lower priority than the false positives
+  // this tracking prevents.
+  // TODO: The ideal solution would be flow-sensitive ownership tracking that
+  // records where values are moved from and to, but this is more complex.
+  if (isStdMove(CE->getDirectCallee()))
+    if (CE->getNumArgs() == 1)
+      if (auto *DRE =
+              dyn_cast<DeclRefExpr>(CE->getArg(0)->IgnoreParenImpCasts()))
+        MovedDecls.insert(DRE->getDecl());
 }
 
 void FactsGenerator::VisitCXXNullPtrLiteralExpr(
@@ -341,6 +359,11 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) {
   // Iterate through all loans to see if any expire.
   for (const auto &Loan : FactMgr.getLoanMgr().getLoans()) {
     const AccessPath &LoanPath = Loan.Path;
+    // Skip loans for declarations that have been moved. When a value is moved,
+    // the original owner no longer has ownership and its destruction should not
+    // cause the loan to expire, preventing false positives.
+    if (MovedDecls.contains(LoanPath.D))
+      continue;
     // Check if the loan is for a stack variable and if that variable
     // is the one being destructed.
     if (LoanPath.D == LifetimeEndsVD)
diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp
index f22c73cfeb784..97a79cc4ce102 100644
--- a/clang/test/Sema/warn-lifetime-safety.cpp
+++ b/clang/test/Sema/warn-lifetime-safety.cpp
@@ -1,9 +1,14 @@
 // RUN: %clang_cc1 -fsyntax-only -fexperimental-lifetime-safety -Wexperimental-lifetime-safety -Wno-dangling -verify %s
 
+#include "Inputs/lifetime-analysis.h"
+
 struct View;
 
 struct [[gsl::Owner]] MyObj {
   int id;
+  MyObj();
+  MyObj(int);
+  MyObj(const MyObj&);
   ~MyObj() {}  // Non-trivial destructor
   MyObj operator+(MyObj);
   
@@ -1297,3 +1302,16 @@ void add(int c, MyObj* node) {
   arr[4] = node;
 }
 } // namespace CppCoverage
+
+namespace do_not_warn_on_std_move {
+void silenced() {
+  MyObj b;
+  View v;
+  {
+    MyObj a;
+    v = a;
+    b = std::move(a); // No warning for 'a' being moved.
+  }
+  (void)v;
+}
+} // namespace do_not_warn_on_std_move

[hokein]

The lifetime safety analysis was previously generating false positives by warning about use-after-lifetime when the original variable was destroyed after being moved. This change prevents those false positives by tracking moved declarations and exempting them from loan expiration checks.

Just a note.

While this fixes false positives, it introduces false negatives. The pointer is not always valid after the owner object is moved, an use-after-free example (when the string uses short string optimization) https://godbolt.org/z/eP7PbaMEn

int main() {
    std::string_view s;
    std::string b;
    {
      std::string a = "12345";  // small literal, stored in the string object, rather than the heap.
      s = a;
      b = std::move(a);
    }
    std::cout << s << "\n"; // oops, s refers to a dangling object.

}

[Xazax-hun]

I think this is a step in the right direction but we should refine this a bit further.

[Xazax-hun]

I am not sure how I feel about this heuristic. People can also do "moves" by not using std::move but casting to an rvalue reference manually, or introducing their own wrapper (that might do some additional checks in some cases).

I think a better way to check if a value was moved whether it was passed to a move constructor. Unfortunately, sometimes we do not see the move ctor call. But we can assume move when the value is passed to a function as an rvalue reference.

So I think I'd rather check if something is passed as an rvalue ref (to move ctor or another function) rather than checking for std::move.

[Xazax-hun]

Is there a way to make this less intrusive? It would be nice if we could disable these checks only for the basic blocks that are reachable from the basic blocks where the move actually happened. So a move at the end of the function should not really prevent us finding bugs at the beginning of the function.

[Xazax-hun]

While this fixes false positives, it introduces false negatives.

I wonder if we should still warn about select classes like std::string in strict mode.