llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-temporal-safety Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Improve lifetime safety analysis by tracking moved objects and providing more precise warnings for potentially false positive cases. - Added support for detecting moves in function calls with rvalue reference parameters - Added a new `MovedOriginFact` to track when objects are moved - Modified the lifetime checker to detect when a loan's storage has been moved - Added new diagnostic messages that indicate when a warning might be a false positive due to moved storage - Added notes in diagnostics to show where objects were potentially moved --- Patch is 38.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/178670.diff 19 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/Checker.h (+1-1) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h (+24) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+4-9) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h (+3-1) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h (+40-1) - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+9-1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+15-2) - (modified) clang/lib/Analysis/LifetimeSafety/Checker.cpp (+37-9) - (modified) clang/lib/Analysis/LifetimeSafety/Dataflow.h (+3) - (modified) clang/lib/Analysis/LifetimeSafety/Facts.cpp (+7) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+32-24) - (modified) clang/lib/Analysis/LifetimeSafety/LifetimeSafety.cpp (+12-11) - (modified) clang/lib/Analysis/LifetimeSafety/LoanPropagation.cpp (+1) - (modified) clang/lib/Analysis/LifetimeSafety/Loans.cpp (+2) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+26-9) - (modified) clang/test/Sema/Inputs/lifetime-analysis.h (+4) - (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+11-10) - (modified) clang/test/Sema/warn-lifetime-safety-dangling-field.cpp (+14-9) - (modified) clang/test/Sema/warn-lifetime-safety.cpp (+37-17) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Checker.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Checker.h index 5c631c92c0167..35e1a1497050d 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Checker.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Checker.h @@ -27,7 +27,7 @@ namespace clang::lifetimes::internal { /// the expired loan. void runLifetimeChecker(const LoanPropagationAnalysis &LoanPropagation, const LiveOriginsAnalysis &LiveOrigins, - const FactManager &FactMgr, AnalysisDeclContext &ADC, + FactManager &FactMgr, AnalysisDeclContext &ADC, LifetimeSafetySemaHelper *SemaHelper); } // namespace clang::lifetimes::internal diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h index d948965af34d5..67a581a593f07 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Facts.h @@ -44,6 +44,8 @@ class Fact { OriginFlow, /// An origin is used (eg. appears as l-value expression like DeclRefExpr). Use, + /// TODO: + MovedOrigins, /// A marker for a specific point in the code, for testing. TestPoint, /// An origin that escapes the function scope (e.g., via return). @@ -219,6 +221,28 @@ class UseFact : public Fact { const OriginManager &OM) const override; }; +/// OriginList of the expression which was found to be moved, e.g, when being +/// used as an argument to an r-value reference parameter. +class MovedOriginFact : public Fact { + const OriginID MovedOrigin; + const Expr *MoveExpr; + +public: + static bool classof(const Fact *F) { + return F->getKind() == Kind::MovedOrigins; + } + + MovedOriginFact(const Expr *MoveExpr, OriginID MovedOrigin) + : Fact(Kind::MovedOrigins), MovedOrigin(MovedOrigin), MoveExpr(MoveExpr) { + } + + OriginID getMovedOrigin() const { return MovedOrigin; } + const Expr *getMoveExpr() const { return MoveExpr; } + + void dump(llvm::raw_ostream &OS, const LoanManager &, + const OriginManager &OM) const override; +}; + /// A dummy-fact used to mark a specific point in the code for testing. /// It is generated by recognizing a `void("__lifetime_test_point_...")` cast. class TestPointFact : public Fact { diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 8b45337bee218..9cb87cb04b8d0 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -67,6 +67,10 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> { void handleGSLPointerConstruction(const CXXConstructExpr *CCE); + /// TODO: Doc. + void handleMovedArgsInCall(const FunctionDecl *FD, + ArrayRef<const Expr *> Args); + /// Checks if a call-like expression creates a borrow by passing a value to a /// reference parameter, creating an IssueFact if it does. /// \param IsGslConstruction True if this is a GSL construction where all @@ -110,15 +114,6 @@ 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/LifetimeSafety.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h index 9f22db20e79b1..1ef2aaa0ff0d9 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeSafety.h @@ -58,16 +58,18 @@ class LifetimeSafetySemaHelper { virtual ~LifetimeSafetySemaHelper() = default; virtual void reportUseAfterFree(const Expr *IssueExpr, const Expr *UseExpr, - SourceLocation FreeLoc, + const Expr *MovedExpr, SourceLocation FreeLoc, Confidence Confidence) {} virtual void reportUseAfterReturn(const Expr *IssueExpr, const Expr *ReturnExpr, + const Expr *MovedExpr, SourceLocation ExpiryLoc, Confidence Confidence) {} virtual void reportDanglingField(const Expr *IssueExpr, const FieldDecl *Field, + const Expr *MovedExpr, SourceLocation ExpiryLoc) {} // Suggests lifetime bound annotations for function paramters. diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h index a366e3e811cbf..385e63de88ad2 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/Loans.h @@ -41,6 +41,12 @@ struct AccessPath { const clang::MaterializeTemporaryExpr *> P; + // An expression which is responsible for moving the storage. Loans with such + // a storage as considered to be potentially moved and could remain valid even + // after the expiry of the access path. Use of such expired loans are reported + // differently as a strict non-default warning. + const Expr *MovedExpr = nullptr; + public: AccessPath(const clang::ValueDecl *D) : P(D) {} AccessPath(const clang::MaterializeTemporaryExpr *MTE) : P(MTE) {} @@ -52,6 +58,15 @@ struct AccessPath { const clang::MaterializeTemporaryExpr *getAsMaterializeTemporaryExpr() const { return P.dyn_cast<const clang::MaterializeTemporaryExpr *>(); } + + bool operator==(const AccessPath &RHS) const { + return getAsValueDecl() == RHS.getAsValueDecl() && + getAsMaterializeTemporaryExpr() == + RHS.getAsMaterializeTemporaryExpr(); + } + + void setMovedExpr(const Expr *MovedExpr) { this->MovedExpr = MovedExpr; } + const Expr *getMovedExpr() const { return MovedExpr; } }; /// An abstract base class for a single "Loan" which represents lending a @@ -96,6 +111,9 @@ class PathLoan : public Loan { const AccessPath &getAccessPath() const { return Path; } const Expr *getIssueExpr() const { return IssueExpr; } + void setMovedExpr(const Expr *MovedExpr) { Path.setMovedExpr(MovedExpr); } + const Expr *getMovedExpr() const { return Path.getMovedExpr(); } + void dump(llvm::raw_ostream &OS) const override; static bool classof(const Loan *L) { return L->getKind() == Kind::Path; } @@ -164,7 +182,28 @@ class LoanManager { assert(ID.Value < AllLoans.size()); return AllLoans[ID.Value]; } + + Loan *getLoan(LoanID ID) { + assert(ID.Value < AllLoans.size()); + return AllLoans[ID.Value]; + } + + const Expr *getMovedExpr(LoanID ID) const { + const Loan *L = getLoan(ID); + if (const PathLoan *PL = dyn_cast<PathLoan>(L)) + return PL->getMovedExpr(); + return nullptr; + } + + void setAsMoved(LoanID ID, const Expr *MovedExpr) { + Loan *L = getLoan(ID); + if (PathLoan *PL = dyn_cast<PathLoan>(L)) + PL->setMovedExpr(MovedExpr); + // TODO: Deal with placeholder loans ? + } + llvm::ArrayRef<const Loan *> getLoans() const { return AllLoans; } + llvm::ArrayRef<Loan *> getLoans() { return AllLoans; } private: LoanID getNextLoanID() { return NextLoanID++; } @@ -172,7 +211,7 @@ class LoanManager { LoanID NextLoanID{0}; /// TODO(opt): Profile and evaluate the usefullness of small buffer /// optimisation. - llvm::SmallVector<const Loan *> AllLoans; + llvm::SmallVector<Loan *> 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 d36ee57fe7651..2f128fda5e31f 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -539,9 +539,17 @@ def LifetimeSafetyDanglingField : DiagGroup<"lifetime-safety-dangling-field"> { }]; } +def LifetimeSafetyDanglingFieldStrict : DiagGroup<"lifetime-safety-dangling-field-strict"> { + code Documentation = [{ + Warning to detect dangling field references. + This may contain false-positives, e.g. when the borrowed storage is potentially moved and is not destroyed at function exit. + }]; +} + def LifetimeSafetyPermissive : DiagGroup<"lifetime-safety-permissive", [LifetimeSafetyDanglingField]>; -def LifetimeSafetyStrict : DiagGroup<"lifetime-safety-strict">; +def LifetimeSafetyStrict : DiagGroup<"lifetime-safety-strict", + [LifetimeSafetyDanglingFieldStrict]>; def LifetimeSafety : DiagGroup<"lifetime-safety", [LifetimeSafetyPermissive, LifetimeSafetyStrict]> { diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 807440c107897..c6484295504a4 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -10816,13 +10816,19 @@ def warn_lifetime_safety_loan_expires_permissive : Warning< def warn_lifetime_safety_loan_expires_strict : Warning< "object whose reference is captured may not live long enough">, InGroup<LifetimeSafetyStrict>, DefaultIgnore; +def warn_lifetime_safety_loan_expires_moved_strict : Warning< + "object whose reference is captured may not live long enough. " + "This could be false positive as the storage may have been moved later">, + InGroup<LifetimeSafetyStrict>, DefaultIgnore; def warn_lifetime_safety_return_stack_addr_permissive : Warning<"address of stack memory is returned later">, InGroup<LifetimeSafetyPermissive>, DefaultIgnore; -def warn_lifetime_safety_return_stack_addr_strict - : Warning<"address of stack memory may be returned later">, +def warn_lifetime_safety_return_stack_addr_moved_strict + : Warning<"address of stack memory may be returned later. " + "This could be false positive as the storage may have been moved. " + "Consider moving first and then aliasing later to resolve the issue">, InGroup<LifetimeSafetyStrict>, DefaultIgnore; @@ -10830,10 +10836,17 @@ def warn_lifetime_safety_dangling_field : Warning<"address of stack memory escapes to a field">, InGroup<LifetimeSafetyDanglingField>, DefaultIgnore; +def warn_lifetime_safety_dangling_field_moved + : Warning<"address of stack memory escapes to a field. " + "This could be a false positive as the storage may have been moved. " + "Consider moving first and then aliasing later to resolve the issue">, + InGroup<LifetimeSafetyDanglingFieldStrict>, + DefaultIgnore; 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 note_lifetime_safety_moved_here : Note<"potentially moved here">; def note_lifetime_safety_dangling_field_here: Note<"this field dangles">; def note_lifetime_safety_escapes_to_field_here: Note<"escapes to this field">; diff --git a/clang/lib/Analysis/LifetimeSafety/Checker.cpp b/clang/lib/Analysis/LifetimeSafety/Checker.cpp index c954a9b14bcdf..ffcbfe85a6a41 100644 --- a/clang/lib/Analysis/LifetimeSafety/Checker.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Checker.cpp @@ -47,6 +47,7 @@ namespace { struct PendingWarning { SourceLocation ExpiryLoc; // Where the loan expired. llvm::PointerUnion<const UseFact *, const OriginEscapesFact *> CausingFact; + const Expr *MovedExpr; Confidence ConfidenceLevel; }; @@ -61,13 +62,13 @@ class LifetimeChecker { llvm::DenseMap<const ParmVarDecl *, EscapingTarget> NoescapeWarningsMap; const LoanPropagationAnalysis &LoanPropagation; const LiveOriginsAnalysis &LiveOrigins; - const FactManager &FactMgr; + FactManager &FactMgr; LifetimeSafetySemaHelper *SemaHelper; ASTContext &AST; public: LifetimeChecker(const LoanPropagationAnalysis &LoanPropagation, - const LiveOriginsAnalysis &LiveOrigins, const FactManager &FM, + const LiveOriginsAnalysis &LiveOrigins, FactManager &FM, AnalysisDeclContext &ADC, LifetimeSafetySemaHelper *SemaHelper) : LoanPropagation(LoanPropagation), LiveOrigins(LiveOrigins), FactMgr(FM), @@ -76,6 +77,8 @@ class LifetimeChecker { for (const Fact *F : FactMgr.getFacts(B)) if (const auto *EF = F->getAs<ExpireFact>()) checkExpiry(EF); + else if (const auto *MOF = F->getAs<MovedOriginFact>()) + handleMovedOrigins(MOF); else if (const auto *OEF = F->getAs<OriginEscapesFact>()) checkAnnotations(OEF); issuePendingWarnings(); @@ -127,6 +130,26 @@ class LifetimeChecker { } } + void handleMovedOrigins(const MovedOriginFact *MOF) { + OriginID OID = MOF->getMovedOrigin(); + // Mark all the access paths of all loans which share the same access path + // with the loans held by the moved origin. + LoanSet MovedLoans = LoanPropagation.getLoans(OID, MOF); + for (LoanID MovedLoan : MovedLoans) { + const Loan *MovedL = FactMgr.getLoanMgr().getLoan(MovedLoan); + const auto *MovedPL = dyn_cast<PathLoan>(MovedL); + if (!MovedPL) + continue; + for (Loan *L : FactMgr.getLoanMgr().getLoans()) { + const auto *PL = dyn_cast<PathLoan>(L); + if (!PL) + continue; + if (PL->getAccessPath() == MovedPL->getAccessPath()) + FactMgr.getLoanMgr().setAsMoved(PL->getID(), MOF->getMoveExpr()); + } + } + } + /// Checks for use-after-free & use-after-return errors when a loan expires. /// /// This method examines all live origins at the expiry point and determines @@ -140,6 +163,8 @@ class LifetimeChecker { /// propagation (e.g., a loan may only be held on some execution paths). void checkExpiry(const ExpireFact *EF) { LoanID ExpiredLoan = EF->getLoanID(); + const Expr *MovedExpr = FactMgr.getLoanMgr().getMovedExpr(ExpiredLoan); + LivenessMap Origins = LiveOrigins.getLiveOriginsAt(EF); Confidence CurConfidence = Confidence::None; // The UseFact or OriginEscapesFact most indicative of a lifetime error, @@ -166,6 +191,7 @@ class LifetimeChecker { return; FinalWarningsMap[ExpiredLoan] = {/*ExpiryLoc=*/EF->getExpiryLoc(), /*BestCausingFact=*/BestCausingFact, + /*MovedExpr=*/MovedExpr, /*ConfidenceLevel=*/CurConfidence}; } @@ -179,19 +205,21 @@ class LifetimeChecker { llvm::PointerUnion<const UseFact *, const OriginEscapesFact *> CausingFact = Warning.CausingFact; Confidence Confidence = Warning.ConfidenceLevel; + const Expr *MovedExpr = Warning.MovedExpr; SourceLocation ExpiryLoc = Warning.ExpiryLoc; if (const auto *UF = CausingFact.dyn_cast<const UseFact *>()) - SemaHelper->reportUseAfterFree(IssueExpr, UF->getUseExpr(), ExpiryLoc, - Confidence); + SemaHelper->reportUseAfterFree(IssueExpr, UF->getUseExpr(), MovedExpr, + ExpiryLoc, Confidence); else if (const auto *OEF = CausingFact.dyn_cast<const OriginEscapesFact *>()) { if (const auto *RetEscape = dyn_cast<ReturnEscapeFact>(OEF)) - SemaHelper->reportUseAfterReturn( - IssueExpr, RetEscape->getReturnExpr(), ExpiryLoc, Confidence); + SemaHelper->reportUseAfterReturn(IssueExpr, + RetEscape->getReturnExpr(), + MovedExpr, ExpiryLoc, Confidence); else if (const auto *FieldEscape = dyn_cast<FieldEscapeFact>(OEF)) SemaHelper->reportDanglingField( - IssueExpr, FieldEscape->getFieldDecl(), ExpiryLoc); + IssueExpr, FieldEscape->getFieldDecl(), MovedExpr, ExpiryLoc); else llvm_unreachable("Unhandled OriginEscapesFact type"); } else @@ -293,8 +321,8 @@ class LifetimeChecker { } // namespace void runLifetimeChecker(const LoanPropagationAnalysis &LP, - const LiveOriginsAnalysis &LO, - const FactManager &FactMgr, AnalysisDeclContext &ADC, + const LiveOriginsAnalysis &LO, FactManager &FactMgr, + AnalysisDeclContext &ADC, LifetimeSafetySemaHelper *SemaHelper) { llvm::TimeTraceScope TimeProfile("LifetimeChecker"); LifetimeChecker Checker(LP, LO, FactMgr, ADC, SemaHelper); diff --git a/clang/lib/Analysis/LifetimeSafety/Dataflow.h b/clang/lib/Analysis/LifetimeSafety/Dataflow.h index 05c20d6385368..eb52e45411b07 100644 --- a/clang/lib/Analysis/LifetimeSafety/Dataflow.h +++ b/clang/lib/Analysis/LifetimeSafety/Dataflow.h @@ -170,6 +170,8 @@ class DataflowAnalysis { return D->transfer(In, *F->getAs<ExpireFact>()); case Fact::Kind::OriginFlow: return D->transfer(In, *F->getAs<OriginFlowFact>()); + case Fact::Kind::MovedOrigins: + return D->transfer(In, *F->getAs<MovedOriginFact>()); case Fact::Kind::OriginEscapes: return D->transfer(In, *F->getAs<OriginEscapesFact>()); case Fact::Kind::Use: @@ -184,6 +186,7 @@ class DataflowAnalysis { Lattice transfer(Lattice In, const IssueFact &) { return In; } Lattice transfer(Lattice In, const ExpireFact &) { return In; } Lattice transfer(Lattice In, const OriginFlowFact &) { return In; } + Lattice transfer(Lattice In, const MovedOriginFact &) { return In; } Lattice transfer(Lattice In, const OriginEscapesFact &) { return In; } Lattice transfer(Lattice In, const UseFact &) { return In; } Lattice transfer(Lattice In, const TestPointFact &) { return In; } diff --git a/clang/lib/Analysis/LifetimeSafety/Facts.cpp b/clang/lib/Analysis/LifetimeSafety/Facts.cpp index 1fc72aa0a4259..bc4000e9ce3ef 100644 --- a/clang/lib/Analysis/LifetimeSafety/Facts.cpp +++ b/clang/lib/Analysis/LifetimeSafety/Facts.cpp @@ -45,6 +45,13 @@ void OriginFlowFact::dump(llvm::raw_ostream &OS, const LoanManager &, OS << "\n"; } +void MovedOriginFact::dump(llvm::raw_ostream &OS, const LoanManager &, + const OriginManager &OM) const { + OS << "MovedOrigins ("; + OM.dump(getMovedOrigin(), OS); + OS << ")\n"; +} + void ReturnEscapeFact::dump(llvm::raw_ostream &OS, const LoanManager &, const OriginManager &OM) const { OS << "OriginEscapes ("; diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index fb859eeb856af..bdb48b0c81172 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -9,6 +9,8 @@ #include <cassert> #include <string> +#include "clang/AST/DeclCXX.h" +#include "clang/AST/ExprCXX.h" #include "clang/AST/OperationKinds.h" #include "clang/Analysis/Analyses/LifetimeSafety/Facts.h" #include "clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h" @@ -185,6 +187,9 @@ void FactsGenerator::VisitCXXConstructExpr(const CXXConstructExpr *CCE) { handleGSLPointerConstruction(CCE); return; ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/178670 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
