llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Add a parameter to `shouldTrackImplicitObjectArg` to differentiate between lifetime safety analysis and statement-local analysis. The statement-local analysis was experiencing false positives when tracking dereference operators for GSL pointers, so this change allows for more aggressive tracking in the full analysis while being more conservative in the statement-local analysis. - Added a `RunningUnderLifetimeSafety` boolean parameter to `shouldTrackImplicitObjectArg` function to distinguish between lifetime safety analysis and Sema's statement-local analysis - Enhanced tracking of dereference operators for GSL pointers in STL: - Now tracks both `operator*` and `operator->` when running under lifetime safety analysis - Avoids tracking these operators in statement-local analysis to prevent false positives --- Full diff: https://github.com/llvm/llvm-project/pull/176794.diff 6 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h (+2-1) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+4-2) - (modified) clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp (+10-5) - (modified) clang/lib/Sema/CheckExprLifetime.cpp (+2-1) - (modified) clang/test/Sema/Inputs/lifetime-analysis.h (+6) - (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+32-2) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h index 760d34d33b15b..3ca7a79d32ee1 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h @@ -51,7 +51,8 @@ bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD); // pointers or references that depend on the lifetime of the object, such as // container iterators (begin, end), data accessors (c_str, data, get), or // element accessors (operator[], operator*, front, back, at). -bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee); +bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, + bool RunningUnderLifetimeSafety); // Returns true if the first argument of a free function should be tracked for // GSL lifetime analysis. This applies to STL free functions that take a pointer diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index 5d1afd15c795d..bbab0627bdead 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -501,7 +501,8 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, if (I == 0) // For the 'this' argument, the attribute is on the method itself. return implicitObjectParamIsLifetimeBound(Method) || - shouldTrackImplicitObjectArg(Method); + shouldTrackImplicitObjectArg( + Method, /*RunningUnderLifetimeSafety=*/true); if ((I - 1) < Method->getNumParams()) // For explicit arguments, find the corresponding parameter // declaration. @@ -520,7 +521,8 @@ void FactsGenerator::handleFunctionCall(const Expr *Call, return false; return I == 0 && isGslPointerType(Method->getFunctionObjectParameterType()) && - shouldTrackImplicitObjectArg(Method); + shouldTrackImplicitObjectArg(Method, + /*RunningUnderLifetimeSafety=*/true); }; if (Args.empty()) return; diff --git a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp index c380543e52a98..93c7a86d0a57c 100644 --- a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp @@ -100,7 +100,8 @@ bool isPointerLikeType(QualType QT) { return isGslPointerType(QT) || QT->isPointerType() || QT->isNullPtrType(); } -bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { +bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee, + bool RunningUnderLifetimeSafety) { if (!Callee) return false; if (auto *Conv = dyn_cast<CXXConversionDecl>(Callee)) @@ -113,10 +114,14 @@ bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { !isGslOwnerType(Callee->getFunctionObjectParameterType())) return false; - // Track dereference operator for GSL pointers in STL. - if (isGslPointerType(Callee->getFunctionObjectParameterType())) - if (Callee->getOverloadedOperator() == OverloadedOperatorKind::OO_Star) - return true; + // Track dereference operator for GSL pointers in STL. Only do so for lifetime + // safety analysis and not for Sema's statement-local analysis as it starts + // to have false-positives. + if (RunningUnderLifetimeSafety && + isGslPointerType(Callee->getFunctionObjectParameterType()) && + (Callee->getOverloadedOperator() == OverloadedOperatorKind::OO_Star || + Callee->getOverloadedOperator() == OverloadedOperatorKind::OO_Arrow)) + return true; if (isPointerLikeType(Callee->getReturnType())) { if (!Callee->getIdentifier()) diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 0b2f00f417337..5d4b4508541b7 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -482,7 +482,8 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, VisitLifetimeBoundArg(Callee, ObjectArg); else if (EnableGSLAnalysis) { if (auto *CME = dyn_cast<CXXMethodDecl>(Callee); - CME && lifetimes::shouldTrackImplicitObjectArg(CME)) + CME && lifetimes::shouldTrackImplicitObjectArg( + CME, /*RunningUnderLifetimeSafety=*/false)) VisitGSLPointerArg(Callee, ObjectArg); } } diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h index b47fe78bdf0fb..4f881d463ebcc 100644 --- a/clang/test/Sema/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/Inputs/lifetime-analysis.h @@ -58,6 +58,12 @@ struct vector { void insert(iterator, T&&); }; +template<typename A, typename B> +struct pair { + A first; + B second; +}; + template<typename T> struct basic_string_view { basic_string_view(); diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index f4d175981619f..5b6a548389bd5 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -986,8 +986,38 @@ std::string_view test_opt_strings(std::optional<std::vector<std::string>> string namespace iterator_arrow { std::string_view test() { std::vector<std::string> strings; - // FIXME: Track operator-> of iterators. - return strings.begin()->data(); + return strings.begin()->data(); // cfg-warning {{address of stack memory is returned later}} cfg-note {{returned here}} +} + +void operator_star_arrow_reference() { + std::vector<std::string> v; + const char* p = v.begin()->data(); + const char* q = (*v.begin()).data(); + const std::string& r = *v.begin(); + + auto temporary = []() { return std::vector<std::string>{{"1"}}; }; + const char* x = temporary().begin()->data(); // cfg-warning {{object whose reference is captured does not live long enough}} cfg-note {{destroyed here}} + const char* y = (*temporary().begin()).data(); // cfg-warning {{object whose reference is captured does not live long enough}} cfg-note {{destroyed here}} + const std::string& z = (*temporary().begin()); // cfg-warning {{object whose reference is captured does not live long enough}} cfg-note {{destroyed here}} + + use(p, q, r, x, y, z); // cfg-note 3 {{later used here}} +} + +void operator_star_arrow_of_iterators_false_positive_no_cfg_analysis() { + std::vector<std::pair<int, std::string>> v; + const char* p = v.begin()->second.data(); + const char* q = (*v.begin()).second.data(); + const std::string& r = (*v.begin()).second; + + // FIXME: Detect this using the CFG-based lifetime analysis. + // Detect dangling references to struct field. + // https://github.com/llvm/llvm-project/issues/176144 + auto temporary = []() { return std::vector<std::pair<int, std::string>>{{1, "1"}}; }; + const char* x = temporary().begin()->second.data(); + const char* y = (*temporary().begin()).second.data(); + const std::string& z = (*temporary().begin()).second; + + use(p, q, r, x, y, z); } } // namespace iterator_arrow `````````` </details> https://github.com/llvm/llvm-project/pull/176794 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
