llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> This PR extends the lifetime safety analysis to handle GSL pointer types (classes marked with `[[gsl::Pointer()]]` attribute), enabling detection of dangling references in pointer-like types such as `string_view` or other view types. Key improvements: - Added detection of borrows created through GSL pointer constructors - Implemented tracking of origin propagation through GSL pointer types - Added support for various initialization patterns (direct, list, functional cast) - Implemented handling of materialized temporaries for GSL pointer types - Added comprehensive test cases for GSL pointer behavior --- Full diff: https://github.com/llvm/llvm-project/pull/154009.diff 3 Files Affected: - (modified) clang/lib/Analysis/LifetimeSafety.cpp (+120-8) - (modified) clang/test/Sema/warn-lifetime-safety.cpp (+110-10) - (modified) clang/unittests/Analysis/LifetimeSafetyTest.cpp (+156-1) ``````````diff diff --git a/clang/lib/Analysis/LifetimeSafety.cpp b/clang/lib/Analysis/LifetimeSafety.cpp index 00efa4c5e548f..f9afa3ff6b5d9 100644 --- a/clang/lib/Analysis/LifetimeSafety.cpp +++ b/clang/lib/Analysis/LifetimeSafety.cpp @@ -433,6 +433,31 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> { void VisitDeclRefExpr(const DeclRefExpr *DRE) { handleUse(DRE); } + void VisitCXXConstructExpr(const CXXConstructExpr *CCE) { + if (!isGslPointerType(CCE->getType())) + return; + + if (CCE->getNumArgs() > 0 && hasOrigin(CCE->getArg(0)->getType())) + // This is a propagation. + addAssignOriginFact(*CCE, *CCE->getArg(0)); + else + // This could be a new borrow. + checkForBorrows(CCE, CCE->getConstructor(), + {CCE->getArgs(), CCE->getNumArgs()}); + } + + void VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { + if (!isGslPointerType(MCE->getImplicitObjectArgument()->getType())) + return; + // Specifically for conversion operators, like `std::string_view p = a;` + if (isa<CXXConversionDecl>(MCE->getCalleeDecl())) { + // The argument is the implicit object itself. + checkForBorrows(MCE, MCE->getMethodDecl(), + {MCE->getImplicitObjectArgument()}); + } + // Note: A more general VisitCallExpr could also be used here. + } + void VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *N) { /// TODO: Handle nullptr expr as a special 'null' loan. Uninitialized /// pointers can use the same type of loan. @@ -492,10 +517,27 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> { void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *FCE) { // Check if this is a test point marker. If so, we are done with this // expression. - if (VisitTestPoint(FCE)) + if (handleTestPoint(FCE)) return; - // Visit as normal otherwise. - Base::VisitCXXFunctionalCastExpr(FCE); + if (isGslPointerType(FCE->getType())) + addAssignOriginFact(*FCE, *FCE->getSubExpr()); + } + + void VisitInitListExpr(const InitListExpr *ILE) { + if (!hasOrigin(ILE->getType())) + return; + // For list initialization with a single element, like `View{...}`, the + // origin of the list itself is the origin of its single element. + if (ILE->getNumInits() == 1) + addAssignOriginFact(*ILE, *ILE->getInit(0)); + } + + void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *MTE) { + if (!hasOrigin(MTE->getType())) + return; + // A temporary object's origin is the same as the origin of the + // expression that initializes it. + addAssignOriginFact(*MTE, *MTE->getSubExpr()); } void handleDestructor(const CFGAutomaticObjDtor &DtorOpt) { @@ -521,8 +563,75 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> { } private: + static bool isGslPointerType(QualType QT) { + if (const auto *RD = QT->getAsCXXRecordDecl()) { + // We need to check the template definition for specializations. + if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) + return CTSD->getSpecializedTemplate() + ->getTemplatedDecl() + ->hasAttr<PointerAttr>(); + return RD->hasAttr<PointerAttr>(); + } + return false; + } + // Check if a type has an origin. - bool hasOrigin(QualType QT) { return QT->isPointerOrReferenceType(); } + static bool hasOrigin(QualType QT) { + if (QT->isFunctionPointerType()) + return false; + return QT->isPointerOrReferenceType() || isGslPointerType(QT); + } + + /// Checks if a call-like expression creates a borrow by passing a local + /// value to a reference parameter, creating an IssueFact if it does. + void checkForBorrows(const Expr *Call, const FunctionDecl *FD, + ArrayRef<const Expr *> Args) { + if (!FD) + return; + + for (unsigned I = 0; I < Args.size(); ++I) { + if (I >= FD->getNumParams()) + break; + + const ParmVarDecl *Param = FD->getParamDecl(I); + const Expr *Arg = Args[I]; + + // This is the core condition for a new borrow: a value type (no origin) + // is passed to a reference parameter. + if (Param->getType()->isReferenceType() && !hasOrigin(Arg->getType())) { + if (const Loan *L = createLoanFrom(Arg, Call)) { + OriginID OID = FactMgr.getOriginMgr().getOrCreate(*Call); + CurrentBlockFacts.push_back( + FactMgr.createFact<IssueFact>(L->ID, OID)); + // For view creation, we assume the first borrow is the significant + // one. + return; + } + } + } + } + + /// Attempts to create a loan by analyzing the source expression of a borrow. + /// This method is the single point for creating loans, allowing for future + /// expansion to handle temporaries, field members, etc. + /// \param SourceExpr The expression representing the object being borrowed + /// from. + /// \param IssueExpr The expression that triggers the borrow (e.g., a + /// constructor call). + /// \return The new Loan on success, nullptr on failure. + const Loan *createLoanFrom(const Expr *SourceExpr, const Expr *IssueExpr) { + // For now, we only handle direct borrows from local variables. + // In the future, this can be extended to handle MaterializeTemporaryExpr, + // etc. + if (const auto *DRE = + dyn_cast<DeclRefExpr>(SourceExpr->IgnoreParenImpCasts())) { + if (const auto *VD = dyn_cast<ValueDecl>(DRE->getDecl())) { + AccessPath Path(VD); + return &FactMgr.getLoanMgr().addLoan(Path, IssueExpr); + } + } + return nullptr; + } template <typename Destination, typename Source> void addAssignOriginFact(const Destination &D, const Source &S) { @@ -534,7 +643,7 @@ class FactGeneratorVisitor : public ConstStmtVisitor<FactGeneratorVisitor> { /// Checks if the expression is a `void("__lifetime_test_point_...")` cast. /// If so, creates a `TestPointFact` and returns true. - bool VisitTestPoint(const CXXFunctionalCastExpr *FCE) { + bool handleTestPoint(const CXXFunctionalCastExpr *FCE) { if (!FCE->getType()->isVoidType()) return false; @@ -612,10 +721,13 @@ class FactGenerator : public RecursiveASTVisitor<FactGenerator> { for (const CFGBlock *Block : *AC.getAnalysis<PostOrderCFGView>()) { FactGeneratorBlockRAII BlockGenerator(FG, Block); for (const CFGElement &Element : *Block) { - if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>()) + if (std::optional<CFGStmt> CS = Element.getAs<CFGStmt>()) { + DEBUG_WITH_TYPE("PrintCFG", llvm::dbgs() << " ================== \n"); + DEBUG_WITH_TYPE("PrintCFG", CS->dump()); + DEBUG_WITH_TYPE("PrintCFG", CS->getStmt()->dumpColor()); TraverseStmt(const_cast<Stmt *>(CS->getStmt())); - else if (std::optional<CFGAutomaticObjDtor> DtorOpt = - Element.getAs<CFGAutomaticObjDtor>()) + } else if (std::optional<CFGAutomaticObjDtor> DtorOpt = + Element.getAs<CFGAutomaticObjDtor>()) FG.handleDestructor(*DtorOpt); } } diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp index 660b9c9d5e243..bc8a5f3f7150f 100644 --- a/clang/test/Sema/warn-lifetime-safety.cpp +++ b/clang/test/Sema/warn-lifetime-safety.cpp @@ -6,6 +6,12 @@ struct MyObj { MyObj operator+(MyObj); }; +struct [[gsl::Pointer()]] View { + View(const MyObj&); // Borrows from MyObj + View(); + void use() const; +}; + //===----------------------------------------------------------------------===// // Basic Definite Use-After-Free (-W...permissive) // These are cases where the pointer is guaranteed to be dangling at the use site. @@ -20,12 +26,31 @@ void definite_simple_case() { (void)*p; // expected-note {{later used here}} } +void definite_simple_case_gsl() { + View v; + { + MyObj s; + v = s; // expected-warning {{object whose reference is captured does not live long enough}} + } // expected-note {{destroyed here}} + v.use(); // expected-note {{later used here}} +} + void no_use_no_error() { MyObj* p; { MyObj s; p = &s; } + // 'p' is dangling here, but since it is never used, no warning is issued. +} + +void no_use_no_error_gsl() { + View v; + { + MyObj s; + v = s; + } + // 'v' is dangling here, but since it is never used, no warning is issued. } void definite_pointer_chain() { @@ -39,6 +64,16 @@ void definite_pointer_chain() { (void)*q; // expected-note {{later used here}} } +void definite_propagation_gsl() { + View v1, v2; + { + MyObj s; + v1 = s; // expected-warning {{object whose reference is captured does not live long enough}} + v2 = v1; + } // expected-note {{destroyed here}} + v2.use(); // expected-note {{later used here}} +} + void definite_multiple_uses_one_warning() { MyObj* p; { @@ -78,6 +113,19 @@ void definite_single_pointer_multiple_loans(bool cond) { (void)*p; // expected-note 2 {{later used here}} } +void definite_single_pointer_multiple_loans_gsl(bool cond) { + View v; + if (cond){ + MyObj s; + v = s; // expected-warning {{object whose reference is captured does not live long enough}} + } // expected-note {{destroyed here}} + else { + MyObj t; + v = t; // expected-warning {{object whose reference is captured does not live long enough}} + } // expected-note {{destroyed here}} + v.use(); // expected-note 2 {{later used here}} +} + //===----------------------------------------------------------------------===// // Potential (Maybe) Use-After-Free (-W...strict) @@ -94,18 +142,14 @@ void potential_if_branch(bool cond) { (void)*p; // expected-note {{later used here}} } -// If all paths lead to a dangle, it becomes a definite error. -void potential_becomes_definite(bool cond) { - MyObj* p; +void potential_if_branch_gsl(bool cond) { + MyObj safe; + View v = safe; if (cond) { - MyObj temp1; - p = &temp1; // expected-warning {{does not live long enough}} - } // expected-note {{destroyed here}} - else { - MyObj temp2; - p = &temp2; // expected-warning {{does not live long enough}} + MyObj temp; + v = temp; // expected-warning {{object whose reference is captured may not live long enough}} } // expected-note {{destroyed here}} - (void)*p; // expected-note 2 {{later used here}} + v.use(); // expected-note {{later used here}} } void definite_potential_together(bool cond) { @@ -159,6 +203,16 @@ void potential_for_loop_use_after_loop_body(MyObj safe) { (void)*p; // expected-note {{later used here}} } +void potential_for_loop_gsl() { + MyObj safe; + View v = safe; + for (int i = 0; i < 1; ++i) { + MyObj s; + v = s; // expected-warning {{object whose reference is captured may not live long enough}} + } // expected-note {{destroyed here}} + v.use(); // expected-note {{later used here}} +} + void potential_for_loop_use_before_loop_body(MyObj safe) { MyObj* p = &safe; for (int i = 0; i < 1; ++i) { @@ -182,6 +236,19 @@ void potential_loop_with_break(bool cond) { (void)*p; // expected-note {{later used here}} } +void potential_loop_with_break_gsl(bool cond) { + MyObj safe; + View v = safe; + for (int i = 0; i < 10; ++i) { + if (cond) { + MyObj temp; + v = temp; // expected-warning {{object whose reference is captured may not live long enough}} + break; // expected-note {{destroyed here}} + } + } + v.use(); // expected-note {{later used here}} +} + void potential_multiple_expiry_of_same_loan(bool cond) { // Choose the last expiry location for the loan. MyObj safe; @@ -258,6 +325,28 @@ void definite_switch(int mode) { (void)*p; // expected-note 3 {{later used here}} } +void definite_switch_gsl(int mode) { + View v; + switch (mode) { + case 1: { + MyObj temp1; + v = temp1; // expected-warning {{object whose reference is captured does not live long enough}} + break; // expected-note {{destroyed here}} + } + case 2: { + MyObj temp2; + v = temp2; // expected-warning {{object whose reference is captured does not live long enough}} + break; // expected-note {{destroyed here}} + } + default: { + MyObj temp3; + v = temp3; // expected-warning {{object whose reference is captured does not live long enough}} + break; // expected-note {{destroyed here}} + } + } + v.use(); // expected-note 3 {{later used here}} +} + //===----------------------------------------------------------------------===// // No-Error Cases //===----------------------------------------------------------------------===// @@ -271,3 +360,14 @@ void no_error_if_dangle_then_rescue() { p = &safe; // p is "rescued" before use. (void)*p; // This is safe. } + +void no_error_if_dangle_then_rescue_gsl() { + MyObj safe; + View v; + { + MyObj temp; + v = temp; // 'v' is temporarily dangling. + } + v = safe; // 'v' is "rescued" before use by reassigning to a valid object. + v.use(); // This is safe. +} diff --git a/clang/unittests/Analysis/LifetimeSafetyTest.cpp b/clang/unittests/Analysis/LifetimeSafetyTest.cpp index 13e5832d70050..98862a1a157b8 100644 --- a/clang/unittests/Analysis/LifetimeSafetyTest.cpp +++ b/clang/unittests/Analysis/LifetimeSafetyTest.cpp @@ -11,7 +11,6 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Testing/TestAST.h" #include "llvm/ADT/StringMap.h" -#include "llvm/Testing/Support/Error.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include <optional> @@ -31,7 +30,13 @@ class LifetimeTestRunner { LifetimeTestRunner(llvm::StringRef Code) { std::string FullCode = R"( #define POINT(name) void("__lifetime_test_point_" #name) + struct MyObj { ~MyObj() {} int i; }; + + struct [[gsl::Pointer()]] View { + View(const MyObj&); + View(); + }; )"; FullCode += Code.str(); @@ -741,5 +746,155 @@ TEST_F(LifetimeAnalysisTest, NoDuplicateLoansForImplicitCastToConst) { EXPECT_THAT(Helper->getLoansForVar("a"), SizeIs(2)); } +TEST_F(LifetimeAnalysisTest, GslPointerSimpleLoan) { + SetupTest(R"( + void target() { + MyObj a; + View x = a; + POINT(p1); + } + )"); + EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerConstructFromOwner) { + SetupTest(R"( + void target() { + MyObj al, bl, cl, dl, el, fl; + View a = View(al); + View b = View{bl}; + View c = View(View(View(cl))); + View d = View{View(View(dl))}; + View e = View{View{View{el}}}; + View f = {fl}; + POINT(p1); + } + )"); + EXPECT_THAT(Origin("a"), HasLoansTo({"al"}, "p1")); + EXPECT_THAT(Origin("b"), HasLoansTo({"bl"}, "p1")); + EXPECT_THAT(Origin("c"), HasLoansTo({"cl"}, "p1")); + EXPECT_THAT(Origin("d"), HasLoansTo({"dl"}, "p1")); + EXPECT_THAT(Origin("e"), HasLoansTo({"el"}, "p1")); + EXPECT_THAT(Origin("f"), HasLoansTo({"fl"}, "p1")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerConstructFromView) { + SetupTest(R"( + void target() { + MyObj a; + View x = View(a); + View y = View{x}; + View z = View(View(View(y))); + View p = View{View(View(x))}; + View q = {x}; + POINT(p1); + } + )"); + EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("y"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("z"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("p"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("q"), HasLoansTo({"a"}, "p1")); +} + +// FIXME: Handle loans in ternary operator! +TEST_F(LifetimeAnalysisTest, GslPointerInConditionalOperator) { + SetupTest(R"( + void target(bool cond) { + MyObj a, b; + View v = cond ? a : b; + POINT(p1); + } + )"); + EXPECT_THAT(Origin("v"), HasLoansTo({}, "p1")); +} + +// FIXME: Handle temporaries. +TEST_F(LifetimeAnalysisTest, ViewFromTemporary) { + SetupTest(R"( + MyObj temporary(); + void target() { + View v = temporary(); + POINT(p1); + } + )"); + EXPECT_THAT(Origin("v"), HasLoansTo({}, "p1")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerWithConstAndAuto) { + SetupTest(R"( + void target() { + MyObj a; + const View v1 = a; + auto v2 = v1; + const auto& v3 = v2; + POINT(p1); + } + )"); + EXPECT_THAT(Origin("v1"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("v2"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("v3"), HasLoansTo({"a"}, "p1")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerPropagation) { + SetupTest(R"( + void target() { + MyObj a; + View x = a; + POINT(p1); + + View y = x; // Propagation via copy-construction + POINT(p2); + + View z; + z = x; // Propagation via copy-assignment + POINT(p3); + } + )"); + + EXPECT_THAT(Origin("x"), HasLoansTo({"a"}, "p1")); + EXPECT_THAT(Origin("y"), HasLoansTo({"a"}, "p2")); + EXPECT_THAT(Origin("z"), HasLoansTo({"a"}, "p3")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerLoanExpiration) { + SetupTest(R"( + void target() { + View x; + { + MyObj a; + x = a; + POINT(before_expiry); + } // `a` is destroyed here. + POINT(after_expiry); + } + )"); + + EXPECT_THAT(NoLoans(), AreExpiredAt("before_expiry")); + EXPECT_THAT(LoansTo({"a"}), AreExpiredAt("after_expiry")); +} + +TEST_F(LifetimeAnalysisTest, GslPointerReassignment) { + SetupTest(R"( + void target() { + MyObj safe; + View v; + v = safe; + POINT(p1); + { + MyObj unsafe; + v = unsafe; + POINT(p2); + } // `unsafe` expires here. + POINT(p3); + } + )"); + + EXPECT_THAT(Origin("v"), HasLoansTo({"safe"}, "p1")); + EXPECT_THAT(Origin("v"), HasLoansTo({"unsafe"}, "p2")); + EXPECT_THAT(Origin("v"), HasLoansTo({"unsafe"}, "p3")); + EXPECT_THAT(LoansTo({"unsafe"}), AreExpiredAt("p3")); +} + } // anonymous namespace } // namespace clang::lifetimes::internal `````````` </details> https://github.com/llvm/llvm-project/pull/154009 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
