Author: Haojian Wu Date: 2024-12-12T20:57:39+01:00 New Revision: 33b910cde3b305a49c98c6de88dbc22ced9dea61
URL: https://github.com/llvm/llvm-project/commit/33b910cde3b305a49c98c6de88dbc22ced9dea61 DIFF: https://github.com/llvm/llvm-project/commit/33b910cde3b305a49c98c6de88dbc22ced9dea61.diff LOG: [clang] Fix the post-filtering heuristic for GSLPointer. (#114044) The lifetime analyzer processes GSL pointers: - when encountering a constructor for a `gsl::pointer`, the analyzer continues traversing the constructor argument, regardless of whether the parameter has a `lifetimebound` annotation. This aims to catch cases where a GSL pointer is constructed from a GSL owner, either directly (e.g., `FooPointer(FooOwner)`) or through a chain of GSL pointers (e.g., `FooPointer(FooPointer(FooOwner))`); - When a temporary object is reported in the callback, the analyzer has heuristics to exclude non-owner types, aiming to avoid false positives (like `FooPointer(FooPointer())`). In the problematic case (discovered in https://github.com/llvm/llvm-project/pull/112751#issuecomment-2441055471) of `return foo.get();`: - When the analyzer reports the local object `foo`, the `Path` is `[GslPointerInit, Lifetimebound]`. - The `Path` goes through [`pathOnlyHandlesGslPointer`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CheckExprLifetime.cpp#L1136) and isn’t filtered out by the [[heuristics]](because `foo` is an owner type), the analyzer treats it as the `FooPointer(FooOwner())` scenario, thus triggering a diagnostic. Filtering out base on the object 'foo' is wrong, because the GSLPointer is constructed from the return result of the `foo.get()`. The patch fixes this by teaching the heuristic to use the return result (only `const GSLOwner&` is considered) of the lifetimebound annotated function. Added: Modified: clang/docs/ReleaseNotes.rst clang/lib/Sema/CheckExprLifetime.cpp clang/test/Sema/Inputs/lifetime-analysis.h clang/test/Sema/warn-lifetime-analysis-nocfg.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 26fa986810a4b8..befa411e882b4c 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -614,6 +614,8 @@ Improvements to Clang's diagnostics - Clang now diagnoses ``[[deprecated]]`` attribute usage on local variables (#GH90073). +- Fix false positives when `[[gsl::Owner/Pointer]]` and `[[clang::lifetimebound]]` are used together. + - Improved diagnostic message for ``__builtin_bit_cast`` size mismatch (#GH115870). - Clang now omits shadow warnings for enum constants in separate class scopes (#GH62588). diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 868081292bc32c..843fdb4a65cd73 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -367,6 +367,8 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { if (Callee->getReturnType()->isReferenceType()) { if (!Callee->getIdentifier()) { auto OO = Callee->getOverloadedOperator(); + if (!Callee->getParent()->hasAttr<OwnerAttr>()) + return false; return OO == OverloadedOperatorKind::OO_Subscript || OO == OverloadedOperatorKind::OO_Star; } @@ -1152,6 +1154,86 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) { } return false; } +// Result of analyzing the Path for GSLPointer. +enum AnalysisResult { + // Path does not correspond to a GSLPointer. + NotGSLPointer, + + // A relevant case was identified. + Report, + // Stop the entire traversal. + Abandon, + // Skip this step and continue traversing inner AST nodes. + Skip, +}; +// Analyze cases where a GSLPointer is initialized or assigned from a +// temporary owner object. +static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path, + Local L) { + if (!pathOnlyHandlesGslPointer(Path)) + return NotGSLPointer; + + // At this point, Path represents a series of operations involving a + // GSLPointer, either in the process of initialization or assignment. + + // Note: A LifetimeBoundCall can appear interleaved in this sequence. + // For example: + // const std::string& Ref(const std::string& a [[clang::lifetimebound]]); + // string_view abc = Ref(std::string()); + // The "Path" is [GSLPointerInit, LifetimeboundCall], where "L" is the + // temporary "std::string()" object. We need to check the return type of the + // function with the lifetimebound attribute. + if (Path.back().Kind == IndirectLocalPathEntry::LifetimeBoundCall) { + // The lifetimebound applies to the implicit object parameter of a method. + const FunctionDecl *FD = + llvm::dyn_cast_or_null<FunctionDecl>(Path.back().D); + // The lifetimebound applies to a function parameter. + if (const auto *PD = llvm::dyn_cast<ParmVarDecl>(Path.back().D)) + FD = llvm::dyn_cast<FunctionDecl>(PD->getDeclContext()); + + if (isa_and_present<CXXConstructorDecl>(FD)) { + // Constructor case: the parameter is annotated with lifetimebound + // e.g., GSLPointer(const S& s [[clang::lifetimebound]]) + // We still respect this case even the type S is not an owner. + return Report; + } + // Check the return type, e.g. + // const GSLOwner& func(const Foo& foo [[clang::lifetimebound]]) + // GSLPointer func(const Foo& foo [[clang::lifetimebound]]) + if (FD && + ((FD->getReturnType()->isReferenceType() && + isRecordWithAttr<OwnerAttr>(FD->getReturnType()->getPointeeType())) || + isPointerLikeType(FD->getReturnType()))) + return Report; + + return Abandon; + } + + if (isa<DeclRefExpr>(L)) { + // We do not want to follow the references when returning a pointer + // originating from a local owner to avoid the following false positive: + // int &p = *localUniquePtr; + // someContainer.add(std::move(localUniquePtr)); + // return p; + if (!pathContainsInit(Path) && isRecordWithAttr<OwnerAttr>(L->getType())) + return Report; + return Abandon; + } + + // The GSLPointer is from a temporary object. + auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L); + + bool IsGslPtrValueFromGslTempOwner = + MTE && !MTE->getExtendingDecl() && + isRecordWithAttr<OwnerAttr>(MTE->getType()); + // Skipping a chain of initializing gsl::Pointer annotated objects. + // We are looking only for the final source to find out if it was + // a local or temporary owner or the address of a local + // variable/param. + if (!IsGslPtrValueFromGslTempOwner) + return Skip; + return Report; +} static bool isAssignmentOperatorLifetimeBound(CXXMethodDecl *CMD) { return CMD && isNormalAssignmentOperator(CMD) && CMD->param_size() == 1 && @@ -1189,27 +1271,17 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity, auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L); - bool IsGslPtrValueFromGslTempOwner = false; - if (pathOnlyHandlesGslPointer(Path)) { - if (isa<DeclRefExpr>(L)) { - // We do not want to follow the references when returning a pointer - // originating from a local owner to avoid the following false positive: - // int &p = *localUniquePtr; - // someContainer.add(std::move(localUniquePtr)); - // return p; - if (pathContainsInit(Path) || - !isRecordWithAttr<OwnerAttr>(L->getType())) - return false; - } else { - IsGslPtrValueFromGslTempOwner = - MTE && !MTE->getExtendingDecl() && - isRecordWithAttr<OwnerAttr>(MTE->getType()); - // Skipping a chain of initializing gsl::Pointer annotated objects. - // We are looking only for the final source to find out if it was - // a local or temporary owner or the address of a local variable/param. - if (!IsGslPtrValueFromGslTempOwner) - return true; - } + bool IsGslPtrValueFromGslTempOwner = true; + switch (analyzePathForGSLPointer(Path, L)) { + case Abandon: + return false; + case Skip: + return true; + case NotGSLPointer: + IsGslPtrValueFromGslTempOwner = false; + LLVM_FALLTHROUGH; + case Report: + break; } switch (LK) { diff --git a/clang/test/Sema/Inputs/lifetime-analysis.h b/clang/test/Sema/Inputs/lifetime-analysis.h index 5c151385b1fe5a..f888e6ab94bb6a 100644 --- a/clang/test/Sema/Inputs/lifetime-analysis.h +++ b/clang/test/Sema/Inputs/lifetime-analysis.h @@ -128,6 +128,11 @@ struct reference_wrapper { template<typename T> reference_wrapper<T> ref(T& t) noexcept; +template <typename T> +struct [[gsl::Pointer]] iterator { + T& operator*() const; +}; + struct false_type { static constexpr bool value = false; constexpr operator bool() const noexcept { return value; } diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index fc876926ba2e63..45b4dc838f44ed 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -604,8 +604,9 @@ struct [[gsl::Pointer]] Span { // Pointer from Owner<Pointer> std::string_view test5() { - std::string_view a = StatusOr<std::string_view>().valueLB(); // expected-warning {{object backing the pointer will be dest}} - return StatusOr<std::string_view>().valueLB(); // expected-warning {{returning address of local temporary}} + // The Owner<Pointer> doesn't own the object which its inner pointer points to. + std::string_view a = StatusOr<std::string_view>().valueLB(); // OK + return StatusOr<std::string_view>().valueLB(); // OK // No dangling diagnostics on non-lifetimebound methods. std::string_view b = StatusOr<std::string_view>().valueNoLB(); @@ -652,7 +653,7 @@ Span<std::string> test10(StatusOr<std::vector<std::string>> aa) { // Pointer<Owner>> from Owner<Pointer<Owner>> Span<std::string> test11(StatusOr<Span<std::string>> aa) { - return aa.valueLB(); // expected-warning {{address of stack memory}} + return aa.valueLB(); // OK return aa.valueNoLB(); // OK. } @@ -693,3 +694,86 @@ void test() { auto y = std::set<int>{}.begin(); // expected-warning {{object backing the pointer}} } } // namespace GH118064 + +namespace LifetimeboundInterleave { + +const std::string& Ref(const std::string& abc [[clang::lifetimebound]]); + +std::string_view TakeSv(std::string_view abc [[clang::lifetimebound]]); +std::string_view TakeStrRef(const std::string& abc [[clang::lifetimebound]]); +std::string_view TakeStr(std::string abc [[clang::lifetimebound]]); + +std::string_view test1() { + std::string_view t1 = Ref(std::string()); // expected-warning {{object backing}} + t1 = Ref(std::string()); // expected-warning {{object backing}} + return Ref(std::string()); // expected-warning {{returning address}} + + std::string_view t2 = TakeSv(std::string()); // expected-warning {{object backing}} + t2 = TakeSv(std::string()); // expected-warning {{object backing}} + return TakeSv(std::string()); // expected-warning {{returning address}} + + std::string_view t3 = TakeStrRef(std::string()); // expected-warning {{temporary}} + t3 = TakeStrRef(std::string()); // expected-warning {{object backing}} + return TakeStrRef(std::string()); // expected-warning {{returning address}} + + + std::string_view t4 = TakeStr(std::string()); + t4 = TakeStr(std::string()); + return TakeStr(std::string()); +} + +template <typename T> +struct Foo { + const T& get() const [[clang::lifetimebound]]; + const T& getNoLB() const; +}; +std::string_view test2(Foo<std::string> r1, Foo<std::string_view> r2) { + std::string_view t1 = Foo<std::string>().get(); // expected-warning {{object backing}} + t1 = Foo<std::string>().get(); // expected-warning {{object backing}} + return r1.get(); // expected-warning {{address of stack}} + + std::string_view t2 = Foo<std::string_view>().get(); + t2 = Foo<std::string_view>().get(); + return r2.get(); + + // no warning on no-LB-annotated method. + std::string_view t3 = Foo<std::string>().getNoLB(); + t3 = Foo<std::string>().getNoLB(); + return r1.getNoLB(); +} + +struct Bar {}; +struct [[gsl::Pointer]] Pointer { + Pointer(const Bar & bar [[clang::lifetimebound]]); +}; +Pointer test3(Bar bar) { + Pointer p = Pointer(Bar()); // expected-warning {{temporary}} + p = Pointer(Bar()); // expected-warning {{object backing}} + return bar; // expected-warning {{address of stack}} +} + +template<typename T> +struct MySpan { + MySpan(const std::vector<T>& v); + using iterator = std::iterator<T>; + iterator begin() const [[clang::lifetimebound]]; +}; +template <typename T> +typename MySpan<T>::iterator ReturnFirstIt(const MySpan<T>& v [[clang::lifetimebound]]); + +void test4() { + std::vector<int> v{1}; + // MySpan<T> doesn't own any underlying T objects, the pointee object of + // the MySpan iterator is still alive when the whole span is destroyed, thus + // no diagnostic. + const int& t1 = *MySpan<int>(v).begin(); + const int& t2 = *ReturnFirstIt(MySpan<int>(v)); + // Ideally, we would diagnose the following case, but due to implementation + // constraints, we do not. + const int& t4 = *MySpan<int>(std::vector<int>{}).begin(); + + 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}} +} + +} // namespace LifetimeboundInterleave _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
