[LifetimeSafety] Introduce AccessPath-based expiry#187708
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-temporal-safety Author: Utkarsh Saxena (usx95) ChangesRefactored the loan system to use access paths instead of loan IDs for expiry tracking, consolidating PathLoan and PlaceholderLoan into a unified Loan class. This is a non-functional refactoring to move towards more granular paths. This also removes a quadratic complexity of Patch is 33.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/187708.diff 10 Files Affected:
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
index fdcf317c69cbf..0f848abd913d3 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h
@@ -102,8 +102,13 @@ class IssueFact : public Fact {
const OriginManager &OM) const override;
};
+/// When an AccessPath expires (e.g., a variable goes out of scope), all loans
+/// that are associated with this path expire. For example, if `x` expires, then
+/// the loan to `x` expires.
class ExpireFact : public Fact {
- LoanID LID;
+ // The access path that expires.
+ AccessPath AP;
+
// Expired origin (e.g., its variable goes out of scope).
std::optional<OriginID> OID;
SourceLocation ExpiryLoc;
@@ -111,11 +116,11 @@ class ExpireFact : public Fact {
public:
static bool classof(const Fact *F) { return F->getKind() == Kind::Expire; }
- ExpireFact(LoanID LID, SourceLocation ExpiryLoc,
+ ExpireFact(AccessPath AP, SourceLocation ExpiryLoc,
std::optional<OriginID> OID = std::nullopt)
- : Fact(Kind::Expire), LID(LID), OID(OID), ExpiryLoc(ExpiryLoc) {}
+ : Fact(Kind::Expire), AP(AP), OID(OID), ExpiryLoc(ExpiryLoc) {}
- LoanID getLoanID() const { return LID; }
+ const AccessPath &getAccessPath() const { return AP; }
std::optional<OriginID> getOriginID() const { return OID; }
SourceLocation getExpiryLoc() const { return ExpiryLoc; }
diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
index 9aaf4627ce5ad..bb3e2cd907e0e 100644
--- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
+++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h
@@ -27,120 +27,96 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, LoanID ID) {
return OS << ID.Value;
}
+/// Represents the base of a placeholder access path, which is either a
+/// function parameter or the implicit 'this' object of an instance method.
+/// Placeholder paths never expire within the function scope, as they represent
+/// storage from the caller's scope.
+class PlaceholderBase {
+ llvm::PointerUnion<const ParmVarDecl *, const CXXMethodDecl *> ParamOrMethod;
+public:
+ PlaceholderBase(const ParmVarDecl *PVD) : ParamOrMethod(PVD) {}
+ PlaceholderBase(const CXXMethodDecl *MD) : ParamOrMethod(MD) {}
+ const ParmVarDecl *getParmVarDecl() const {
+ return ParamOrMethod.dyn_cast<const ParmVarDecl *>();
+ }
+ const CXXMethodDecl *getMethodDecl() const {
+ return ParamOrMethod.dyn_cast<const CXXMethodDecl *>();
+ }
+};
+
/// Represents the storage location being borrowed, e.g., a specific stack
-/// variable.
-/// TODO: Model access paths of other types, e.g., s.field, heap and globals.
+/// variable or a field within it: var.field.*
+///
+/// An AccessPath consists of:
+/// - A base: either a ValueDecl, MaterializeTemporaryExpr, or PlaceholderBase
+/// - A sequence of PathElements representing field accesses or interior
+/// regions
+///
+/// Examples:
+/// - `x` -> Base=x, Elements=[]
+/// - `x.field` -> Base=x, Elements=[.field]
+/// - `x.*` (e.g., string_view from string) -> Base=x, Elements=[.*]
+/// - `x.field.*` -> Base=x, Elements=[.field, .*]
+/// - `$param.field` -> Base=$param, Elements=[.field]
+///
+/// TODO: Model access paths of other types, e.g. heap and globals.
class AccessPath {
- // An access path can be:
- // - ValueDecl * , to represent the storage location corresponding to the
- // variable declared in ValueDecl.
- // - MaterializeTemporaryExpr * , to represent the storage location of the
- // temporary object materialized via this MaterializeTemporaryExpr.
+ /// The base of the access path: a variable, temporary, or placeholder.
const llvm::PointerUnion<const clang::ValueDecl *,
- const clang::MaterializeTemporaryExpr *>
- P;
-
+ const clang::MaterializeTemporaryExpr *,
+ const PlaceholderBase *>
+ Base;
public:
- AccessPath(const clang::ValueDecl *D) : P(D) {}
- AccessPath(const clang::MaterializeTemporaryExpr *MTE) : P(MTE) {}
-
+ AccessPath(const clang::ValueDecl *D) : Base(D) {}
+ AccessPath(const clang::MaterializeTemporaryExpr *MTE) : Base(MTE) {}
+ AccessPath(const PlaceholderBase *PB) : Base(PB) {}
+ /// Creates an extended access path by appending a path element.
+ /// Example: AccessPath(x_path, field) creates path to `x.field`.
+ AccessPath(const AccessPath &Other)
+ : Base(Other.Base){
+ }
const clang::ValueDecl *getAsValueDecl() const {
- return P.dyn_cast<const clang::ValueDecl *>();
+ return Base.dyn_cast<const clang::ValueDecl *>();
}
-
const clang::MaterializeTemporaryExpr *getAsMaterializeTemporaryExpr() const {
- return P.dyn_cast<const clang::MaterializeTemporaryExpr *>();
+ return Base.dyn_cast<const clang::MaterializeTemporaryExpr *>();
}
-
- bool operator==(const AccessPath &RHS) const { return P == RHS.P; }
+ const PlaceholderBase *getAsPlaceholderBase() const {
+ return Base.dyn_cast<const PlaceholderBase *>();
+ }
+ bool operator==(const AccessPath &RHS) const {
+ return Base == RHS.Base;
+ }
+ bool operator!=(const AccessPath &RHS) const {
+ return !(Base == RHS.Base);
+ }
+ void dump(llvm::raw_ostream &OS) const;
};
-/// An abstract base class for a single "Loan" which represents lending a
-/// storage in memory.
+/// Represents lending a storage location.
+//
+/// A loan tracks the borrowing relationship created by operations like
+/// taking a pointer/reference (&x), creating a view (std::string_view sv = s),
+/// or receiving a parameter.
+///
+/// Examples:
+/// - `int* p = &x;` creates a loan to `x`
+/// - `std::string_view v = s;` creates a loan to `s.*` (interior) [FIXME]
+/// - `int* p = &obj.field;` creates a loan to `obj.field` [FIXME]
+/// - Parameter loans have no IssueExpr (created at function entry)
class Loan {
- /// TODO: Represent opaque loans.
- /// TODO: Represent nullptr: loans to no path. Accessing it UB! Currently it
- /// is represented as empty LoanSet
-public:
- enum class Kind : uint8_t {
- /// A loan with an access path to a storage location.
- Path,
- /// A non-expiring placeholder loan for a parameter, representing a borrow
- /// from the function's caller.
- Placeholder
- };
-
- Loan(Kind K, LoanID ID) : K(K), ID(ID) {}
- virtual ~Loan() = default;
-
- Kind getKind() const { return K; }
- LoanID getID() const { return ID; }
-
- virtual void dump(llvm::raw_ostream &OS) const = 0;
-
-private:
- const Kind K;
const LoanID ID;
-};
-
-/// PathLoan represents lending a storage location that is visible within the
-/// function's scope (e.g., a local variable on stack).
-class PathLoan : public Loan {
- AccessPath Path;
- /// The expression that creates the loan, e.g., &x.
+ const AccessPath Path;
+ /// The expression that creates the loan, e.g., &x. Null for placeholder
+ /// loans.
const Expr *IssueExpr;
-
public:
- PathLoan(LoanID ID, AccessPath Path, const Expr *IssueExpr)
- : Loan(Kind::Path, ID), Path(Path), IssueExpr(IssueExpr) {}
-
+ Loan(LoanID ID, AccessPath Path, const Expr *IssueExpr = nullptr)
+ : ID(ID), Path(Path), IssueExpr(IssueExpr) {}
+ LoanID getID() const { return ID; }
const AccessPath &getAccessPath() const { return Path; }
const Expr *getIssueExpr() const { return IssueExpr; }
-
- void dump(llvm::raw_ostream &OS) const override;
-
- static bool classof(const Loan *L) { return L->getKind() == Kind::Path; }
-};
-
-/// A placeholder loan held by a function parameter or an implicit 'this'
-/// object, representing a borrow from the caller's scope.
-///
-/// Created at function entry for each pointer or reference parameter or for
-/// the implicit 'this' parameter of instance methods, with an
-/// origin. Unlike PathLoan, placeholder loans:
-/// - Have no IssueExpr (created at function entry, not at a borrow site)
-/// - Have no AccessPath (the borrowed object is not visible to the function)
-/// - Do not currently expire, but may in the future when modeling function
-/// invalidations (e.g., vector::push_back)
-///
-/// When a placeholder loan escapes the function (e.g., via return), it
-/// indicates the parameter or method should be marked [[clang::lifetimebound]],
-/// enabling lifetime annotation suggestions.
-class PlaceholderLoan : public Loan {
- /// The function parameter or method (representing 'this') that holds this
- /// placeholder loan.
- llvm::PointerUnion<const ParmVarDecl *, const CXXMethodDecl *> ParamOrMethod;
-
-public:
- PlaceholderLoan(LoanID ID, const ParmVarDecl *PVD)
- : Loan(Kind::Placeholder, ID), ParamOrMethod(PVD) {}
-
- PlaceholderLoan(LoanID ID, const CXXMethodDecl *MD)
- : Loan(Kind::Placeholder, ID), ParamOrMethod(MD) {}
-
- const ParmVarDecl *getParmVarDecl() const {
- return ParamOrMethod.dyn_cast<const ParmVarDecl *>();
- }
-
- const CXXMethodDecl *getMethodDecl() const {
- return ParamOrMethod.dyn_cast<const CXXMethodDecl *>();
- }
-
- void dump(llvm::raw_ostream &OS) const override;
-
- static bool classof(const Loan *L) {
- return L->getKind() == Kind::Placeholder;
- }
+ void dump(llvm::raw_ostream &OS) const;
};
/// Manages the creation, storage and retrieval of loans.
@@ -148,23 +124,23 @@ class LoanManager {
public:
LoanManager() = default;
- template <typename LoanType, typename... Args>
- LoanType *createLoan(Args &&...args) {
- static_assert(
- std::is_same_v<LoanType, PathLoan> ||
- std::is_same_v<LoanType, PlaceholderLoan>,
- "createLoan can only be used with PathLoan or PlaceholderLoan");
- void *Mem = LoanAllocator.Allocate<LoanType>();
- auto *NewLoan =
- new (Mem) LoanType(getNextLoanID(), std::forward<Args>(args)...);
+ Loan *createLoan(AccessPath Path, const Expr *IssueExpr = nullptr) {
+ void *Mem = LoanAllocator.Allocate<Loan>();
+ auto *NewLoan = new (Mem) Loan(getNextLoanID(), Path, IssueExpr);
AllLoans.push_back(NewLoan);
return NewLoan;
+
}
const Loan *getLoan(LoanID ID) const {
assert(ID.Value < AllLoans.size());
return AllLoans[ID.Value];
}
+
+ /// Gets or creates a placeholder base for a given parameter or method.
+ const PlaceholderBase *getOrCreatePlaceholderBase(const ParmVarDecl *PVD);
+ const PlaceholderBase *getOrCreatePlaceholderBase(const CXXMethodDecl *MD);
+
llvm::ArrayRef<const Loan *> getLoans() const { return AllLoans; }
private:
@@ -174,6 +150,7 @@ class LoanManager {
/// TODO(opt): Profile and evaluate the usefullness of small buffer
/// optimisation.
llvm::SmallVector<const Loan *> AllLoans;
+ llvm::DenseMap<const Decl*, const PlaceholderBase*> PlaceholderBases;
llvm::BumpPtrAllocator LoanAllocator;
};
} // namespace clang::lifetimes::internal
diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp
index db87f511a230f..86c7d5c70558f 100644
--- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp
@@ -139,60 +139,47 @@ class LifetimeChecker {
};
for (LoanID LID : EscapedLoans) {
const Loan *L = FactMgr.getLoanMgr().getLoan(LID);
- const auto *PL = dyn_cast<PlaceholderLoan>(L);
- if (!PL)
+ const PlaceholderBase *PB = L->getAccessPath().getAsPlaceholderBase();
+ if (!PB)
continue;
- if (const auto *PVD = PL->getParmVarDecl())
+ if (const auto *PVD = PB->getParmVarDecl())
CheckParam(PVD);
- else if (const auto *MD = PL->getMethodDecl())
+ else if (const auto *MD = PB->getMethodDecl())
CheckImplicitThis(MD);
}
}
- /// Checks for use-after-free & use-after-return errors when a loan expires.
+ /// Checks for use-after-free & use-after-return errors when an access path
+ /// expires (e.g., a variable goes out of scope).
///
- /// This method examines all live origins at the expiry point and determines
- /// if any of them hold the expiring loan. If so, it creates a pending
- /// warning.
- ///
- /// Note: This implementation considers only the confidence of origin
- /// liveness. Future enhancements could also consider the confidence of loan
- /// propagation (e.g., a loan may only be held on some execution paths).
+ /// When a path expires, all loans having this path expires.
+ /// This method examines all live origins and reports warnings for loans they
+ /// hold that are prefixed by the expired path.
void checkExpiry(const ExpireFact *EF) {
- LoanID ExpiredLoan = EF->getLoanID();
- const Expr *MovedExpr = nullptr;
- if (auto *ME = MovedLoans.getMovedLoans(EF).lookup(ExpiredLoan))
- MovedExpr = *ME;
-
+ const AccessPath &ExpiredPath = EF->getAccessPath();
LivenessMap Origins = LiveOrigins.getLiveOriginsAt(EF);
- bool CurDomination = false;
- // The UseFact or OriginEscapesFact most indicative of a lifetime error,
- // prioritized by earlier source location.
- llvm::PointerUnion<const UseFact *, const OriginEscapesFact *> CausingFact =
- nullptr;
-
for (auto &[OID, LiveInfo] : Origins) {
LoanSet HeldLoans = LoanPropagation.getLoans(OID, EF);
- if (!HeldLoans.contains(ExpiredLoan))
- continue;
- // Loan is defaulted.
-
- if (!CurDomination || causingFactDominatesExpiry(LiveInfo.Kind))
- CausingFact = LiveInfo.CausingFact;
+ for (LoanID HeldLoanID : HeldLoans) {
+ const Loan *HeldLoan = FactMgr.getLoanMgr().getLoan(HeldLoanID);
+ if (ExpiredPath != HeldLoan->getAccessPath())
+ continue;
+ // HeldLoan is expired because its AccessPath is expired.
+ PendingWarning &CurWarning = FinalWarningsMap[HeldLoan->getID()];
+ const Expr *MovedExpr = nullptr;
+ if (auto *ME = MovedLoans.getMovedLoans(EF).lookup(HeldLoanID))
+ MovedExpr = *ME;
+ // Skip if we already have a dominating causing fact.
+ if (CurWarning.CausingFactDominatesExpiry)
+ continue;
+ if (causingFactDominatesExpiry(LiveInfo.Kind))
+ CurWarning.CausingFactDominatesExpiry = true;
+ CurWarning.CausingFact = LiveInfo.CausingFact;
+ CurWarning.ExpiryLoc = EF->getExpiryLoc();
+ CurWarning.MovedExpr = MovedExpr;
+ CurWarning.InvalidatedByExpr = nullptr;
+ }
}
- if (!CausingFact)
- return;
-
- bool LastDomination =
- FinalWarningsMap.lookup(ExpiredLoan).CausingFactDominatesExpiry;
- if (LastDomination)
- return;
- FinalWarningsMap[ExpiredLoan] = {
- /*ExpiryLoc=*/EF->getExpiryLoc(),
- /*CausingFact=*/CausingFact,
- /*MovedExpr=*/MovedExpr,
- /*InvalidatedByExpr=*/nullptr,
- /*CausingFactDominatesExpiry=*/CurDomination};
}
/// Checks for use-after-invalidation errors when a container is modified.
@@ -206,18 +193,9 @@ class LifetimeChecker {
LoanSet DirectlyInvalidatedLoans =
LoanPropagation.getLoans(InvalidatedOrigin, IOF);
auto IsInvalidated = [&](const Loan *L) {
- auto *PathL = dyn_cast<PathLoan>(L);
- auto *PlaceholderL = dyn_cast<PlaceholderLoan>(L);
for (LoanID InvalidID : DirectlyInvalidatedLoans) {
- const Loan *L = FactMgr.getLoanMgr().getLoan(InvalidID);
- auto *InvalidPathL = dyn_cast<PathLoan>(L);
- auto *InvalidPlaceholderL = dyn_cast<PlaceholderLoan>(L);
- if (PathL && InvalidPathL &&
- PathL->getAccessPath() == InvalidPathL->getAccessPath())
- return true;
- if (PlaceholderL && InvalidPlaceholderL &&
- PlaceholderL->getParmVarDecl() ==
- InvalidPlaceholderL->getParmVarDecl())
+ const Loan *InvalidL = FactMgr.getLoanMgr().getLoan(InvalidID);
+ if (InvalidL->getAccessPath() == L->getAccessPath())
return true;
}
return false;
@@ -248,13 +226,10 @@ class LifetimeChecker {
return;
for (const auto &[LID, Warning] : FinalWarningsMap) {
const Loan *L = FactMgr.getLoanMgr().getLoan(LID);
-
- const Expr *IssueExpr = nullptr;
- if (const auto *BL = dyn_cast<PathLoan>(L))
- IssueExpr = BL->getIssueExpr();
+ const Expr *IssueExpr = L->getIssueExpr();
const ParmVarDecl *InvalidatedPVD = nullptr;
- if (const auto *PL = dyn_cast<PlaceholderLoan>(L))
- InvalidatedPVD = PL->getParmVarDecl();
+ if (const PlaceholderBase *PB = L->getAccessPath().getAsPlaceholderBase())
+ InvalidatedPVD = PB->getParmVarDecl();
llvm::PointerUnion<const UseFact *, const OriginEscapesFact *>
CausingFact = Warning.CausingFact;
const Expr *MovedExpr = Warning.MovedExpr;
@@ -263,24 +238,30 @@ class LifetimeChecker {
if (const auto *UF = CausingFact.dyn_cast<const UseFact *>()) {
if (Warning.InvalidatedByExpr) {
if (IssueExpr)
+ // Use-after-invalidation of an object on stack.
SemaHelper->reportUseAfterInvalidation(IssueExpr, UF->getUseExpr(),
Warning.InvalidatedByExpr);
- if (InvalidatedPVD)
+ else if (InvalidatedPVD)
+ // Use-after-invalidation of a parameter.
SemaHelper->reportUseAfterInvalidation(
InvalidatedPVD, UF->getUseExpr(), Warning.InvalidatedByExpr);
} else
+ // Scope-based expiry (use-after-scope).
SemaHelper->reportUseAfterFree(IssueExpr, UF->getUseExpr(), MovedExpr,
ExpiryLoc);
} else if (const auto *OEF =
CausingFact.dyn_cast<const OriginEscapesFact *>()) {
if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF))
+ // Return stack address.
SemaHelper->reportUseAfterReturn(
IssueExpr, RetEscape->getReturnExpr(), MovedExpr, ExpiryLoc);
else if (const auto *FieldEscape = dyn_cast<FieldEscapeFact>(OEF))
+ // Dangling field.
SemaHelper->reportDanglingField(
IssueExpr, FieldEscape->getFieldDecl(), MovedExpr, ExpiryLoc);
else if (const auto *GlobalEscape = dyn_cast<GlobalEscapeFact>(OEF))
+ // Global escape.
SemaHelper->reportDanglingGlobal(IssueExpr, GlobalEscape->getGlobal(),
MovedExpr, ExpiryLoc);
else
diff --git a/clang/lib/Analysis/LifetimeSafety/Facts.cpp b/clang/lib/Analysis/LifetimeSafety/Facts.cpp
index 535da2a014273..1bc0521a72359 100644
--- a/clang/lib/Analysis/LifetimeSafety/Facts.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/Facts.cpp
@@ -8,10 +8,7 @@
#include "clang/Analysis/Analyses/LifetimeSafety/Facts.h"
#include "clang/AST/Decl.h"
-#include "clang/AST/DeclID.h"
#include "clang/Analysis/Analyses/PostOrderCFGView.h"
-#include "clang/Analysis/FlowSensitive/DataflowWorklist.h"
-#include "llvm/ADT/STLFunctionalExtras.h"
namespace clang::lifetimes::internal {
@@ -32,7 +29,7 @@ void IssueFact::dump(llvm::raw_ostream &OS, const LoanManager &LM,
void ExpireFact::dump(llvm::raw_ostream &OS, const LoanManager &LM,
const OriginManager &OM) const {
OS << "Expire (";
- LM.getLoan(getLoanID())->dump(OS);
+ getAccessPath().dump(OS);
if (auto OID = getOriginID()) {
OS << ", Origin: ";
OM.dump(*OID, OS);
diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
index 3259505584c9f..42cc4d17c39da 100644
--- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
+++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp
@@ -69,12 +69,11 @@ void FactsGenerator::flow(OriginList *Dst, OriginList *Src, bool Kill) {
/// This function should be called whenever a DeclRefExpr represents a borrow.
/// \param DRE The declaration reference expression that initiates the borrow.
/// \return The new Loan on success, nullptr otherwise.
-static const PathLoan *createLoan(FactManager &FactMgr,
- const DeclRefExpr *DRE) {
+static const Loan *createLoan(FactManager &FactMgr, const DeclRefExpr *DRE) {
if (const auto *VD = dyn_cast<ValueDecl>(DRE->getDecl())...
[truncated]
|
f2cd3ea to
eaf270c
Compare
Xazax-hun
left a comment
There was a problem hiding this comment.
Overall looks good to me, some nits and one bigger question inline.
eaf270c to
4e555f3
Compare
Xazax-hun
left a comment
There was a problem hiding this comment.
This looks great, thanks! I have one more question inline.
4e555f3 to
b2269f4
Compare
Xazax-hun
left a comment
There was a problem hiding this comment.
I love this! This opens up doors for new features and simplifies the code at the same time.
|
I will land this for now. I expect it to reduce compile-times so will report back the latest profiles. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/16584 Here is the relevant piece of the build log for the reference |
Refactored the loan system to use access paths instead of loan IDs for expiry tracking, consolidating PathLoan and PlaceholderLoan into a unified Loan class. This is a non-functional refactoring to move towards more granular paths. This also removes a quadratic complexity of `handleLifetimeEnds` where we iterated over all loans to find which loans expired.

Refactored the loan system to use access paths instead of loan IDs for expiry tracking, consolidating PathLoan and PlaceholderLoan into a unified Loan class.
This is a non-functional refactoring to move towards more granular paths. This also removes a quadratic complexity of
handleLifetimeEndswhere we iterated over all loans to find which loans expired.