llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Improve lifetime analysis for STL iterators in range-based `for` loops by tracking dereference operator of GSL pointers. Added support for dereference operators (`*`) for GSL pointers in STL to track pointer value instead of `this` arg reference. Tests: - Removed a test which started to fail and didn't look correctly annotated (?). - Added new test cases for range-based for loop variables and iterator arrow operators ## **Thoughts on overall direction:** I feel we are twisting clang too much here to hardcode heuristics which cannot be otherwise expressed through available annotations. This had initially started by the intention of implicitly annotating STL to benefit every implementation out there. But this has moved to a direction where we have heuristics which cannot be anymore expressed by annotations (like transparent accessors and inner pointers). This is also a fallout of making [[clang::lifetimebound]] and [[gsl::*]] annotations work together. Some takeaways for me here are: 1. It is great to have inferred annotations for STL but they should be expressible through explicit annotations especially when STL implementers are willing to add these (e.g. https://github.com/llvm/llvm-project/pull/112751). 2. We need a simple way to express transparent accessor functions of GSL pointers. ### Transparent functions: ```cpp int* transparent(int* in) { return in; } ``` There is no "outlives" constraint imposed by this function. Instead, it introduces only an aliasing effect. The return value is an alias to `in`. This aliasing effect can be expressed through `clang::lifetimebound`. ```cpp int* transparent(int* in [[clang::lifetimebound]]) { return in; } ``` Same is true for `view` types. ```cpp // return view aliases 'in' which points to some char* owned by some 'std::string'. // This helps in upholding the contract "that std::string should outlive the value returned here". std::string_view transparent(std::string_view in [[clang::lifetimebound]]) { return in; } ``` This gets complicated when we have references to `view` types. ```cpp // Not possible to annotate this to express the same effect/contract. std::string_view transparent2(const std::string_view& in) { return in; } ``` This gets much harder when we want to express this for accessor methods of view types as implicit this arg is always a reference. ```cpp class [[gsl::Pointer]] MySpan { MySpan(MyVector<int>& v [[clang::lifetimebound]]); // 'MySpan' should not outlive 'v'. Ok. // 'begin' aliases MySpan which aliases 'v'. No way to express this. const MyVector* begin() const { return begin; } const MyVector* end() const { return end; } private: const MyVector* begin; const MyVector* end; }; // Again possible to annotate free methods provided view types is accepted as a value. const MyVector* getBegin(MySpan my_span [[clang::lifetimebound]]) { // returns the pointee of the pointer 'my_span'. return my_span.begin(); } ``` The problematic cases are quite similar though: ```cpp std::string_view transparent2(const std::string_view& in) { return in; } class [[gsl::Pointer]] MySpan { const MyVector* begin() const { return begin; } }; ``` These essentially talk about **inner** pointer. For `transparent2`, we want to express that it aliases the inner pointer of `const std::string_view&`. For `MySpan::begin()`, we want to the express that it aliases the inner pointer of `const MySpan&` type which is the type of implicit `this` parameter. ### Options: - `[[clang::lifetimebound(2)]]`: Refer to the inner pointer after peeling the outer pointer/reference. More generally `[[clang::lifetimebound(x)]]` where `x` is a positive integer to peel off `x` layers of pointers. cc: @<!-- -->Xazax-hun --- Full diff: https://github.com/llvm/llvm-project/pull/176643.diff 2 Files Affected: - (modified) clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp (+7) - (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+53-5) ``````````diff diff --git a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp index 2772fe20de19b..9843930999a0f 100644 --- a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp @@ -99,6 +99,13 @@ bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { if (!isGslPointerType(Callee->getFunctionObjectParameterType()) && !isGslOwnerType(Callee->getFunctionObjectParameterType())) return false; + + // Track dereference operator for GSL pointers in STL. + if (isGslPointerType(Callee->getFunctionObjectParameterType())) + if (const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(Callee)) + if (MD->getOverloadedOperator() == OverloadedOperatorKind::OO_Star) + return true; + if (isPointerLikeType(Callee->getReturnType())) { if (!Callee->getIdentifier()) return false; diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 7fdc493dbd17a..d7410125f55bb 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -911,10 +911,13 @@ struct MySpan { MySpan(const std::vector<T>& v); ~MySpan(); using iterator = std::iterator<T>; - iterator begin() const [[clang::lifetimebound]]; + // FIXME: It is not possible to annotate accessor methods of non-owning view types. + // Clang should provide another annotation to mark such functions as 'transparent'. + iterator begin() const; }; +// FIXME: Same as above. template <typename T> -typename MySpan<T>::iterator ReturnFirstIt(const MySpan<T>& v [[clang::lifetimebound]]); +typename MySpan<T>::iterator ReturnFirstIt(const MySpan<T>& v); void test4() { std::vector<int> v{1}; @@ -926,15 +929,60 @@ void test4() { // Ideally, we would diagnose the following case, but due to implementation // constraints, we do not. const int& t4 = *MySpan<int>(std::vector<int>{}).begin(); + use(t1, t2, t4); - // FIXME: Detect this using the CFG-based lifetime analysis (constructor of a pointer). - auto it1 = MySpan<int>(v).begin(); // expected-warning {{temporary whose address is use}} - auto it2 = ReturnFirstIt(MySpan<int>(v)); // expected-warning {{temporary whose address is used}} + auto it1 = MySpan<int>(v).begin(); + auto it2 = ReturnFirstIt(MySpan<int>(v)); use(it1, it2); } } // namespace LifetimeboundInterleave +namespace range_based_for_loop_variables { +std::string_view test_view_loop_var(std::vector<std::string> strings) { + for (std::string_view s : strings) { // cfg-warning {{address of stack memory is returned later}} + return s; //cfg-note {{returned here}} + } + return ""; +} + +const char* test_view_loop_var_with_data(std::vector<std::string> strings) { + for (std::string_view s : strings) { // cfg-warning {{address of stack memory is returned later}} + return s.data(); //cfg-note {{returned here}} + } + return ""; +} + +std::string_view test_no_error_for_views(std::vector<std::string_view> views) { + for (std::string_view s : views) { + return s; + } + return ""; +} + +std::string_view test_string_ref_var(std::vector<std::string> strings) { + for (const std::string& s : strings) { // cfg-warning {{address of stack memory is returned later}} + return s; //cfg-note {{returned here}} + } + return ""; +} + +std::string_view test_opt_strings(std::optional<std::vector<std::string>> strings_or) { + for (const std::string& s : *strings_or) { // cfg-warning {{address of stack memory is returned later}} + return s; //cfg-note {{returned here}} + } + return ""; +} +} // namespace range_based_for_loop_variables + +namespace iterator_arrow { +std::string_view test() { + std::vector<std::string> strings; + // FIXME: Track operator-> of iterators. + return strings.begin()->data(); +} +} // namespace iterator_arrow + namespace GH120206 { struct S { std::string_view s; `````````` </details> https://github.com/llvm/llvm-project/pull/176643 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
