diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 878cb90b685f9..a1acd8615afdd 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -91,6 +91,7 @@ class FactsGenerator : public ConstStmtVisitor { void markUseAsWrite(const DeclRefExpr *DRE); + llvm::SmallVector issuePlaceholderLoans(); FactManager &FactMgr; AnalysisDeclContext &AC; llvm::SmallVector CurrentBlockFacts; diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index b34a7f18b5809..90d69e766ec78 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -47,6 +47,9 @@ class LifetimeSafetyReporter { const Expr *EscapeExpr, SourceLocation ExpiryLoc, Confidence Confidence) {} + + virtual void suggestAnnotation(const ParmVarDecl *PVD, + const Expr *EscapeExpr) {} }; /// The main entry point for the analysis. diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h index 7f5cf03fd3e5f..63628959d7d80 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h @@ -34,21 +34,80 @@ struct AccessPath { AccessPath(const clang::ValueDecl *D) : D(D) {} }; -/// Information about a single borrow, or "Loan". A loan is created when a -/// reference or pointer is created. -struct Loan { +/// An abstract base class for a single borrow, or "Loan". +class Loan { /// TODO: Represent opaque loans. /// TODO: Represent nullptr: loans to no path. Accessing it UB! Currently it /// is represented as empty LoanSet - LoanID ID; +public: + enum class Kind : uint8_t { + /// A regular borrow of a variable within the function that has a path and + /// can expire. + Borrow, + /// 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; +}; + +/// Information about a single borrow, or "Loan". A loan is created when a +/// reference or pointer is created. +class BorrowLoan : public Loan { AccessPath Path; - /// The expression that creates the loan, e.g., &x. const Expr *IssueExpr; - Loan(LoanID id, AccessPath path, const Expr *IssueExpr) - : ID(id), Path(path), IssueExpr(IssueExpr) {} +public: + BorrowLoan(LoanID ID, AccessPath Path, const Expr *IssueExpr) + : Loan(Kind::Borrow, ID), Path(Path), IssueExpr(IssueExpr) {} + + 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::Borrow; } +}; + +/// A placeholder loan held by a function parameter, representing a borrow from +/// the caller's scope. +/// +/// Created at function entry for each pointer or reference parameter with an +/// origin. Unlike BorrowLoan, 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 should be marked [[clang::lifetimebound]], enabling +/// lifetime annotation suggestions. +class PlaceholderLoan : public Loan { + /// The function parameter that holds this placeholder loan. + const ParmVarDecl *PVD; - void dump(llvm::raw_ostream &OS) const; +public: + PlaceholderLoan(LoanID ID, const ParmVarDecl *PVD) + : Loan(Kind::Placeholder, ID), PVD(PVD) {} + + const ParmVarDecl *getParmVarDecl() const { return PVD; } + + void dump(llvm::raw_ostream &OS) const override; + + static bool classof(const Loan *L) { + return L->getKind() == Kind::Placeholder; + } }; /// Manages the creation, storage and retrieval of loans. @@ -56,16 +115,20 @@ class LoanManager { public: LoanManager() = default; - Loan &addLoan(AccessPath Path, const Expr *IssueExpr) { - AllLoans.emplace_back(getNextLoanID(), Path, IssueExpr); - return AllLoans.back(); + template + LoanType *createLoan(Args &&...args) { + void *Mem = LoanAllocator.Allocate(); + auto *NewLoan = + new (Mem) LoanType(getNextLoanID(), std::forward(args)...); + AllLoans.push_back(NewLoan); + return NewLoan; } - const Loan &getLoan(LoanID ID) const { + const Loan *getLoan(LoanID ID) const { assert(ID.Value < AllLoans.size()); return AllLoans[ID.Value]; } - llvm::ArrayRef getLoans() const { return AllLoans; } + llvm::ArrayRef getLoans() const { return AllLoans; } private: LoanID getNextLoanID() { return NextLoanID++; } @@ -73,7 +136,8 @@ class LoanManager { LoanID NextLoanID{0}; /// TODO(opt): Profile and evaluate the usefullness of small buffer /// optimisation. - llvm::SmallVector AllLoans; + llvm::SmallVector AllLoans; + llvm::BumpPtrAllocator LoanAllocator; }; } // namespace clang::lifetimes::internal diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 2fff32bbc4d6c..d2537870bfd64 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -541,6 +541,8 @@ def LifetimeSafety : DiagGroup<"experimental-lifetime-safety", Experimental warnings to detect use-after-free and related temporal safety bugs based on lifetime safety analysis. }]; } +def LifetimeSafetySuggestions + : DiagGroup<"experimental-lifetime-safety-suggestions">; def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">; def DllexportExplicitInstantiationDecl : DiagGroup<"dllexport-explicit-instantiation-decl">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 4a145fd71eedd..a9e70ba731c87 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -10778,6 +10778,13 @@ def note_lifetime_safety_used_here : Note<"later used here">; def note_lifetime_safety_destroyed_here : Note<"destroyed here">; def note_lifetime_safety_returned_here : Note<"returned here">; +def warn_lifetime_safety_suggest_lifetimebound + : Warning<"param should be marked [[clang::lifetimebound]]">, + InGroup, + DefaultIgnore; + +def note_lifetime_safety_suggestion_returned_here : Note<"param returned here">; + // For non-floating point, expressions of the form x == x or x != x // should result in a warning, since these always evaluate to a constant. // Array comparisons have similar warnings diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index 1f7c282dadac2..9faac34f4874c 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -50,6 +50,7 @@ struct PendingWarning { class LifetimeChecker { private: llvm::DenseMap FinalWarningsMap; + llvm::DenseMap AnnotationWarningsMap; const LoanPropagationAnalysis &LoanPropagation; const LiveOriginsAnalysis &LiveOrigins; const FactManager &FactMgr; @@ -65,7 +66,26 @@ class LifetimeChecker { for (const Fact *F : FactMgr.getFacts(B)) if (const auto *EF = F->getAs()) checkExpiry(EF); + else if (const auto *OEF = F->getAs()) + checkAnnotations(OEF); issuePendingWarnings(); + issueAnnotationWarnings(); + } + + /// Checks if an escaping origin holds a placeholder loan, indicating a + /// missing [[clang::lifetimebound]] annotation. + void checkAnnotations(const OriginEscapesFact *OEF) { + OriginID EscapedOID = OEF->getEscapedOriginID(); + LoanSet EscapedLoans = LoanPropagation.getLoans(EscapedOID, OEF); + for (LoanID LID : EscapedLoans) { + const Loan *L = FactMgr.getLoanMgr().getLoan(LID); + if (const auto *PL = dyn_cast(L)) { + const ParmVarDecl *PVD = PL->getParmVarDecl(); + if (PVD->hasAttr()) + continue; + AnnotationWarningsMap.try_emplace(PVD, OEF->getEscapeExpr()); + } + } } /// Checks for use-after-free & use-after-return errors when a loan expires. @@ -114,8 +134,9 @@ class LifetimeChecker { if (!Reporter) return; for (const auto &[LID, Warning] : FinalWarningsMap) { - const Loan &L = FactMgr.getLoanMgr().getLoan(LID); - const Expr *IssueExpr = L.IssueExpr; + const Loan *L = FactMgr.getLoanMgr().getLoan(LID); + const auto *BL = cast(L); + const Expr *IssueExpr = BL->getIssueExpr(); llvm::PointerUnion CausingFact = Warning.CausingFact; Confidence Confidence = Warning.ConfidenceLevel; @@ -132,6 +153,13 @@ class LifetimeChecker { llvm_unreachable("Unhandled CausingFact type"); } } + + void issueAnnotationWarnings() { + if (!Reporter) + return; + for (const auto &[PVD, EscapeExpr] : AnnotationWarningsMap) + Reporter->suggestAnnotation(PVD, EscapeExpr); + } }; } // namespace diff --git a/clang/lib/Analysis/LifetimeSafety/Facts.cpp b/clang/lib/Analysis/LifetimeSafety/Facts.cpp index 0ae7111c489e8..68317318ff4e2 100644 --- a/clang/lib/Analysis/LifetimeSafety/Facts.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Facts.cpp @@ -20,7 +20,7 @@ void Fact::dump(llvm::raw_ostream &OS, const LoanManager &, void IssueFact::dump(llvm::raw_ostream &OS, const LoanManager &LM, const OriginManager &OM) const { OS << "Issue ("; - LM.getLoan(getLoanID()).dump(OS); + LM.getLoan(getLoanID())->dump(OS); OS << ", ToOrigin: "; OM.dump(getOriginID(), OS); OS << ")\n"; @@ -29,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 &) const { OS << "Expire ("; - LM.getLoan(getLoanID()).dump(OS); + LM.getLoan(getLoanID())->dump(OS); OS << ")\n"; } diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 00870c3fd4086..988c99860f5ce 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -31,22 +31,28 @@ static bool hasOrigin(const VarDecl *VD) { /// 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 Loan *createLoan(FactManager &FactMgr, const DeclRefExpr *DRE) { +static const BorrowLoan *createLoan(FactManager &FactMgr, + const DeclRefExpr *DRE) { if (const auto *VD = dyn_cast(DRE->getDecl())) { AccessPath Path(VD); // The loan is created at the location of the DeclRefExpr. - return &FactMgr.getLoanMgr().addLoan(Path, DRE); + return FactMgr.getLoanMgr().createLoan(Path, DRE); } return nullptr; } void FactsGenerator::run() { llvm::TimeTraceScope TimeProfile("FactGenerator"); + const CFG &Cfg = *AC.getCFG(); + llvm::SmallVector PlaceholderLoanFacts = issuePlaceholderLoans(); // Iterate through the CFG blocks in reverse post-order to ensure that // initializations and destructions are processed in the correct sequence. for (const CFGBlock *Block : *AC.getAnalysis()) { CurrentBlockFacts.clear(); EscapesInCurrentBlock.clear(); + if (Block == &Cfg.getEntry()) + CurrentBlockFacts.append(PlaceholderLoanFacts.begin(), + PlaceholderLoanFacts.end()); for (unsigned I = 0; I < Block->size(); ++I) { const CFGElement &Element = Block->Elements[I]; if (std::optional CS = Element.getAs()) @@ -85,7 +91,7 @@ void FactsGenerator::VisitDeclRefExpr(const DeclRefExpr *DRE) { if (const Loan *L = createLoan(FactMgr, DRE)) { OriginID ExprOID = FactMgr.getOriginMgr().getOrCreate(*DRE); CurrentBlockFacts.push_back( - FactMgr.createFact(L->ID, ExprOID)); + FactMgr.createFact(L->getID(), ExprOID)); } } } @@ -223,13 +229,14 @@ void FactsGenerator::handleLifetimeEnds(const CFGLifetimeEnds &LifetimeEnds) { if (!LifetimeEndsVD) return; // Iterate through all loans to see if any expire. - for (const auto &Loan : FactMgr.getLoanMgr().getLoans()) { - const AccessPath &LoanPath = Loan.Path; - // Check if the loan is for a stack variable and if that variable - // is the one being destructed. - if (LoanPath.D == LifetimeEndsVD) - CurrentBlockFacts.push_back(FactMgr.createFact( - Loan.ID, LifetimeEnds.getTriggerStmt()->getEndLoc())); + for (const auto *Loan : FactMgr.getLoanMgr().getLoans()) { + if (const auto *BL = dyn_cast(Loan)) { + // Check if the loan is for a stack variable and if that variable + // is the one being destructed. + if (BL->getAccessPath().D == LifetimeEndsVD) + CurrentBlockFacts.push_back(FactMgr.createFact( + BL->getID(), LifetimeEnds.getTriggerStmt()->getEndLoc())); + } } } @@ -342,4 +349,24 @@ void FactsGenerator::markUseAsWrite(const DeclRefExpr *DRE) { UseFacts[DRE]->markAsWritten(); } +// Creates an IssueFact for a new placeholder loan for each pointer or reference +// parameter at the function's entry. +llvm::SmallVector FactsGenerator::issuePlaceholderLoans() { + const auto *FD = dyn_cast(AC.getDecl()); + if (!FD) + return {}; + + llvm::SmallVector PlaceholderLoanFacts; + for (const ParmVarDecl *PVD : FD->parameters()) { + if (hasOrigin(PVD)) { + const PlaceholderLoan *L = + FactMgr.getLoanMgr().createLoan(PVD); + OriginID OID = FactMgr.getOriginMgr().getOrCreate(*PVD); + PlaceholderLoanFacts.push_back( + FactMgr.createFact(L->getID(), OID)); + } + } + return PlaceholderLoanFacts; +} + } // namespace clang::lifetimes::internal diff --git a/clang/lib/Analysis/LifetimeSafety/Loans.cpp b/clang/lib/Analysis/LifetimeSafety/Loans.cpp index 2c85a3c6083f3..8398cd790b03c 100644 --- a/clang/lib/Analysis/LifetimeSafety/Loans.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Loans.cpp @@ -10,9 +10,13 @@ namespace clang::lifetimes::internal { -void Loan::dump(llvm::raw_ostream &OS) const { - OS << ID << " (Path: "; +void BorrowLoan::dump(llvm::raw_ostream &OS) const { + OS << getID() << " (Path: "; OS << Path.D->getNameAsString() << ")"; } +void PlaceholderLoan::dump(llvm::raw_ostream &OS) const { + OS << getID() << " (Placeholder loan)"; +} + } // namespace clang::lifetimes::internal diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 43d2b9a829545..c952578b79d80 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2884,6 +2884,19 @@ class LifetimeSafetyReporterImpl : public LifetimeSafetyReporter { << EscapeExpr->getEndLoc(); } + void suggestAnnotation(const ParmVarDecl *PVD, + const Expr *EscapeExpr) override { + SourceLocation InsertionPoint = Lexer::getLocForEndOfToken( + PVD->getEndLoc(), 0, S.getSourceManager(), S.getLangOpts()); + S.Diag(PVD->getBeginLoc(), diag::warn_lifetime_safety_suggest_lifetimebound) + << PVD->getSourceRange() + << FixItHint::CreateInsertion(InsertionPoint, + " [[clang::lifetimebound]]"); + S.Diag(EscapeExpr->getBeginLoc(), + diag::note_lifetime_safety_suggestion_returned_here) + << EscapeExpr->getSourceRange(); + } + private: Sema &S; }; diff --git a/clang/test/Sema/warn-lifetime-safety-suggestions.cpp b/clang/test/Sema/warn-lifetime-safety-suggestions.cpp new file mode 100644 index 0000000000000..0a2f1e1de9b15 --- /dev/null +++ b/clang/test/Sema/warn-lifetime-safety-suggestions.cpp @@ -0,0 +1,84 @@ +// RUN: %clang_cc1 -fsyntax-only -fexperimental-lifetime-safety -Wexperimental-lifetime-safety-suggestions -verify %s + +struct MyObj { + int id; + ~MyObj() {} // Non-trivial destructor + MyObj operator+(MyObj); +}; + +struct [[gsl::Pointer()]] View { + View(const MyObj&); // Borrows from MyObj + View(); + void use() const; +}; + +View return_view_directly (View a) { // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + return a; // expected-note {{param returned here}} +} + +View conditional_return_view ( + View a, // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + View b, // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + bool c) { + View res; + if (c) + res = a; + else + res = b; + return res; // expected-note 2 {{param returned here}} +} + +// FIXME: Fails to generate lifetime suggestion for reference types as these are not handled currently. +MyObj& return_reference (MyObj& a, MyObj& b, bool c) { + if(c) { + return a; + } + return b; +} + +// FIXME: Fails to generate lifetime suggestion for reference types as these are not handled currently. +View return_view_from_reference (MyObj& p) { + return p; +} + +int* return_pointer_directly (int* a) { // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + return a; // expected-note {{param returned here}} +} + +MyObj* return_pointer_object (MyObj* a) { // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + return a; // expected-note {{param returned here}} +} + +View only_one_paramter_annotated (View a [[clang::lifetimebound]], + View b, // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + bool c) { + if(c) + return a; + return b; // expected-note {{param returned here}} +} + +View reassigned_to_another_parameter ( + View a, + View b) { // expected-warning {{param should be marked [[clang::lifetimebound]]}}. + a = b; + return a; // expected-note {{param returned here}} +} + +//===----------------------------------------------------------------------===// +// Negative Test Cases +//===----------------------------------------------------------------------===// + +View already_annotated(View a [[clang::lifetimebound]]) { + return a; +} + +MyObj return_obj_by_value(MyObj& p) { + return p; +} + +MyObj GlobalMyObj; +View Global = GlobalMyObj; +View Reassigned(View a) { + a = Global; + return a; +} diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp index a895475013c98..22824e4bf05db 100644 --- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp +++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp @@ -114,9 +114,11 @@ class LifetimeTestHelper { return {}; } std::vector LID; - for (const Loan &L : Analysis.getFactManager().getLoanMgr().getLoans()) - if (L.Path.D == VD) - LID.push_back(L.ID); + for (const Loan *L : Analysis.getFactManager().getLoanMgr().getLoans()) + if (const auto *BL = dyn_cast(L)) { + if (BL->getAccessPath().D == VD) + LID.push_back(L->getID()); + } if (LID.empty()) { ADD_FAILURE() << "Loan for '" << VarName << "' not found."; return {};