Author: Zhijie Wang Date: 2026-05-25T14:21:35-07:00 New Revision: d0646cbe307dd2bd10686f923061a7d981263e40
URL: https://github.com/llvm/llvm-project/commit/d0646cbe307dd2bd10686f923061a7d981263e40 DIFF: https://github.com/llvm/llvm-project/commit/d0646cbe307dd2bd10686f923061a7d981263e40.diff LOG: [LifetimeSafety] Fix false negative for GSL Owner methods inherited from a non-Owner base (#197864) - Take the implicit object's actual type (e.g., the type before any `DerivedToBase` cast) into account when checking for GSL Owner. Other `isGslOwnerType` call sites with the same pattern (e.g., `isGslOwnerType(MCE->getImplicitObjectArgument()->getType())` in `VisitCXXMemberCallExpr`) lack a real-world trigger today and are deferred to a follow-up. - Unify the GSL Owner checks inside `shouldTrackImplicitObjectArg` so they share a single source of truth. Fixes: #188832 Added: Modified: clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp clang/lib/Sema/CheckExprLifetime.cpp clang/test/Sema/warn-lifetime-safety.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h index ec05e67b05853..a97df7a08dfeb 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h @@ -62,7 +62,8 @@ bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD); // container iterators (begin, end), data accessors (c_str, data, get), // element accessors (operator[], operator*, front, back, at), or propagating // operations (operator+, operator-, operator++, operator--). -bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, +bool shouldTrackImplicitObjectArg(const Expr &ImplicitObjectArgument, + const CXXMethodDecl *Callee, bool RunningUnderLifetimeSafety); // Returns true if the first argument of a free function should be tracked for @@ -81,6 +82,7 @@ bool shouldTrackSecondArgument(const FunctionDecl *FD); bool isGslPointerType(QualType QT); // Tells whether the type is annotated with [[gsl::Owner]]. bool isGslOwnerType(QualType QT); +bool isGslOwnerType(const CXXRecordDecl *RD); // Returns true if the given method is std::unique_ptr::release(). // This is treated as a move in lifetime analysis to avoid false-positives diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 582a8fd1645a8..9038f56689779 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -886,7 +886,7 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, handleImplicitObjectFieldUses(Call, FD); if (!CallList) return; - auto IsArgLifetimeBound = [FD](unsigned I) -> bool { + auto IsArgLifetimeBound = [FD, &Args](unsigned I) -> bool { const ParmVarDecl *PVD = nullptr; if (const auto *Method = dyn_cast<CXXMethodDecl>(FD); Method && Method->isInstance() && !isa<CXXConstructorDecl>(FD)) { @@ -894,7 +894,7 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, // For the 'this' argument, the attribute is on the method itself. return implicitObjectParamIsLifetimeBound(Method) || shouldTrackImplicitObjectArg( - Method, /*RunningUnderLifetimeSafety=*/true); + *Args[0], Method, /*RunningUnderLifetimeSafety=*/true); if ((I - 1) < Method->getNumParams()) // For explicit arguments, find the corresponding parameter // declaration. @@ -909,13 +909,13 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, } return PVD ? PVD->hasAttr<clang::LifetimeBoundAttr>() : false; }; - auto shouldTrackPointerImplicitObjectArg = [FD](unsigned I) -> bool { + auto shouldTrackPointerImplicitObjectArg = [FD, &Args](unsigned I) -> bool { const auto *Method = dyn_cast<CXXMethodDecl>(FD); if (!Method || !Method->isInstance()) return false; return I == 0 && isGslPointerType(Method->getFunctionObjectParameterType()) && - shouldTrackImplicitObjectArg(Method, + shouldTrackImplicitObjectArg(*Args[0], Method, /*RunningUnderLifetimeSafety=*/true); }; if (Args.empty()) diff --git a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp index 903edfbab5489..2f26e77d5a0eb 100644 --- a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp @@ -128,16 +128,22 @@ static bool isReferenceOrPointerLikeType(QualType QT) { return QT->isReferenceType() || isPointerLikeType(QT); } -bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, +bool shouldTrackImplicitObjectArg(const Expr &ImplicitObjectArgument, + const CXXMethodDecl *Callee, bool RunningUnderLifetimeSafety) { if (!Callee) return false; + // Check both the declaring class and the call-site object: a gsl::Owner + // may inherit its accessors from a non-Owner base (e.g. libc++ optional). + const bool IsGslOwnerImplicitObject = + isGslOwnerType(Callee->getFunctionObjectParameterType()) || + (RunningUnderLifetimeSafety && + isGslOwnerType(ImplicitObjectArgument.getBestDynamicClassType())); if (auto *Conv = dyn_cast<CXXConversionDecl>(Callee)) - if (isGslPointerType(Conv->getConversionType()) && - Callee->getParent()->hasAttr<OwnerAttr>()) + if (isGslPointerType(Conv->getConversionType()) && IsGslOwnerImplicitObject) return true; if (!isGslPointerType(Callee->getFunctionObjectParameterType()) && - !isGslOwnerType(Callee->getFunctionObjectParameterType())) + !IsGslOwnerImplicitObject) return false; // Begin and end iterators. @@ -181,7 +187,7 @@ bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, if (!Callee->getIdentifier()) // e.g., std::optional<T>::operator->() returns T*. return RunningUnderLifetimeSafety - ? Callee->getParent()->hasAttr<OwnerAttr>() && + ? IsGslOwnerImplicitObject && Callee->getOverloadedOperator() == OverloadedOperatorKind::OO_Arrow : false; @@ -192,7 +198,7 @@ bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, if (Callee->getReturnType()->isReferenceType()) { if (!Callee->getIdentifier()) { auto OO = Callee->getOverloadedOperator(); - if (!Callee->getParent()->hasAttr<OwnerAttr>()) + if (!IsGslOwnerImplicitObject) return false; return OO == OverloadedOperatorKind::OO_Subscript || OO == OverloadedOperatorKind::OO_Star; @@ -272,8 +278,7 @@ bool shouldTrackSecondArgument(const FunctionDecl *FD) { !isa<CXXMethodDecl>(FD); } -template <typename T> static bool isRecordWithAttr(QualType Type) { - auto *RD = Type->getAsCXXRecordDecl(); +template <typename T> static bool isRecordWithAttr(const CXXRecordDecl *RD) { if (!RD) return false; // Generally, if a primary template class declaration is annotated with an @@ -299,8 +304,15 @@ template <typename T> static bool isRecordWithAttr(QualType Type) { return Result; } +template <typename T> static bool isRecordWithAttr(QualType Type) { + return isRecordWithAttr<T>(Type->getAsCXXRecordDecl()); +} + bool isGslPointerType(QualType QT) { return isRecordWithAttr<PointerAttr>(QT); } bool isGslOwnerType(QualType QT) { return isRecordWithAttr<OwnerAttr>(QT); } +bool isGslOwnerType(const CXXRecordDecl *RD) { + return isRecordWithAttr<OwnerAttr>(RD); +} static StringRef getName(const CXXRecordDecl &RD) { if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(&RD)) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 4e050e9bf6045..d15b2c6518cec 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -483,7 +483,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, else if (EnableGSLAnalysis) { if (auto *CME = dyn_cast<CXXMethodDecl>(Callee); CME && lifetimes::shouldTrackImplicitObjectArg( - CME, /*RunningUnderLifetimeSafety=*/false)) + *ObjectArg, CME, /*RunningUnderLifetimeSafety=*/false)) VisitGSLPointerArg(Callee, ObjectArg); } } diff --git a/clang/test/Sema/warn-lifetime-safety.cpp b/clang/test/Sema/warn-lifetime-safety.cpp index 70ec968dfd5c1..0619fda738fa4 100644 --- a/clang/test/Sema/warn-lifetime-safety.cpp +++ b/clang/test/Sema/warn-lifetime-safety.cpp @@ -3332,3 +3332,47 @@ void assign_non_capturing_to_function_ref(function_ref &r) { } } // namespace GH126600 + +namespace GH188832 { +inline namespace __1 { +template <class T> +struct __optional_storage_base { + T& operator*() &; + T* operator->(); + T& value() &; +}; + +template <class T> +struct [[gsl::Owner]] optional : __optional_storage_base<T> { + ~optional(); +}; +} // namespace __1 + +const MyObj& return_optional_deref() { + optional<MyObj> opt; + return *opt; // expected-warning {{address of stack memory is returned later}} \ + // expected-note {{returned here}} +} + +const MyObj& return_optional_value() { + optional<MyObj> opt; + return opt.value(); // expected-warning {{address of stack memory is returned later}} \ + // expected-note {{returned here}} +} + +const int* return_optional_arrow() { + optional<MyObj> opt; + return &opt->id; // expected-warning {{address of stack memory is returned later}} \ + // expected-note {{returned here}} +} + +void deref_use_after_scope() { + const MyObj* p; + { + optional<MyObj> opt; + p = &*opt; // expected-warning {{object whose reference is captured does not live long enough}} + } // expected-note {{destroyed here}} + (void)p->id; // expected-note {{later used here}} +} + +} // namespace GH188832 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
