https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/170007
>From ba4333864015b116357492dbf8dadb497bc4ea5c Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <[email protected]> Date: Sat, 29 Nov 2025 16:43:06 +0000 Subject: [PATCH] std_move false positive --- .../Analyses/LifetimeSafety/FactsGenerator.h | 9 +++++ .../Analysis/Analyses/LifetimeSafety/Loans.h | 2 + .../LifetimeSafety/FactsGenerator.cpp | 19 +++++++++ clang/test/Sema/warn-lifetime-safety.cpp | 39 +++++++++++++++++++ 4 files changed, 69 insertions(+) diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 0e23dc8ea0fc5..ee2f1aae4a4c5 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -106,6 +106,15 @@ 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; + + // 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. + llvm::DenseSet<const ValueDecl *> MovedDecls; }; } // namespace clang::lifetimes::internal diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h index 6dfe4c8fa6b9c..ee1634a6f5ea2 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h @@ -30,6 +30,7 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, LoanID ID) { /// variable. /// TODO: Model access paths of other types, e.g., s.field, heap and globals. struct AccessPath { +private: // An access path can be: // - ValueDecl * , to represent the storage location corresponding to the // variable declared in ValueDecl. @@ -39,6 +40,7 @@ struct AccessPath { const clang::MaterializeTemporaryExpr *> P; +public: AccessPath(const clang::ValueDecl *D) : P(D) {} AccessPath(const clang::MaterializeTemporaryExpr *MTE) : P(MTE) {} diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index bb82f09fa8457..8b25356298dad 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -203,9 +203,23 @@ 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. We do not + // generate expire facts for moved decls to avoid false alarms. + 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( @@ -395,6 +409,11 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { // Iterate through all loans to see if any expire. for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) { if (const auto *BL = dyn_cast<PathLoan>(Loan)) { + // 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(BL->getAccessPath().getAsValueDecl())) + continue; // Check if the loan is for a stack variable and if that variable // is the one being destructed. const AccessPath AP = BL->getAccessPath(); diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp index 6535245853c7f..0b1962b7cb651 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); @@ -1392,3 +1397,37 @@ 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; +} + +void silenced_flow_insensitive(bool c) { + MyObj a; + View v = a; + if (c) { + MyObj b = std::move(a); + } + (void)v; +} + +// FIXME: Silence when move arg is not a declref. +void take(MyObj&&); +void not_silenced_via_conditional(bool cond) { + View v; + { + MyObj a, b; + v = cond ? a : b; // expected-warning 2 {{object whose reference }} + take(std::move(cond ? a : b)); + } // expected-note 2 {{destroyed here}} + (void)v; // expected-note 2 {{later used here}} +} +} // namespace do_not_warn_on_std_move _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
