Skip to content

[LifetimeSafety] Track moved declarations to prevent false positives #170007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
usx95 wants to merge 1 commit into
base: users/usx95/11-29-dereference_operator
Choose a base branch
from
+46 −0
Open

[LifetimeSafety] Track moved declarations to prevent false positives #170007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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.
Copy link
Collaborator

@ymand ymand Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving the comments on lines 195-201 here, or at least mentioning them. But, that's a matter of taste :)

Also, whether in comments or test maybe highlight some example impacts? e.g. modifying your test below:

{
  MyObj a;
  v = a;
  b = std::move(a);

  refresh(a); // For types which can take on valid state after move.
  v = a;
}

Or, a branch?

{
  MyObj a;
  v = a;
  if (c) {
    b = std::move(a);
  }
}

llvm::DenseSet<const ValueDecl *> MovedDecls;
};

} // namespace clang::lifetimes::internal
Expand Down
23 changes: 23 additions & 0 deletions clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,27 @@ void FactsGenerator::VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
}
}

static bool isStdMove(const FunctionDecl *FD) {
Copy link
Collaborator

@Xazax-hun Xazax-hun Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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(
Expand Down Expand Up @@ -364,6 +382,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().D))
continue;
// Check if the loan is for a stack variable and if that variable
// is the one being destructed.
if (BL->getAccessPath().D == LifetimeEndsVD)
Expand Down
18 changes: 18 additions & 0 deletions clang/test/Sema/warn-lifetime-safety.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);

Expand Down Expand Up @@ -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
Loading