diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 26fa986810a4b..befa411e882b4 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 868081292bc32..843fdb4a65cd7 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()) + 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(Path.back().D); + // The lifetimebound applies to a function parameter. + if (const auto *PD = llvm::dyn_cast(Path.back().D)) + FD = llvm::dyn_cast(PD->getDeclContext()); + + if (isa_and_present(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(FD->getReturnType()->getPointeeType())) || + isPointerLikeType(FD->getReturnType()))) + return Report; + + return Abandon; + } + + if (isa(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(L->getType())) + return Report; + return Abandon; + } + + // The GSLPointer is from a temporary object. + auto *MTE = dyn_cast(L); + + bool IsGslPtrValueFromGslTempOwner = + MTE && !MTE->getExtendingDecl() && + isRecordWithAttr(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(L); - bool IsGslPtrValueFromGslTempOwner = false; - if (pathOnlyHandlesGslPointer(Path)) { - if (isa(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(L->getType())) - return false; - } else { - IsGslPtrValueFromGslTempOwner = - MTE && !MTE->getExtendingDecl() && - isRecordWithAttr(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 5c151385b1fe5..f888e6ab94bb6 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 reference_wrapper ref(T& t) noexcept; +template +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 fc876926ba2e6..45b4dc838f44e 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 std::string_view test5() { - std::string_view a = StatusOr().valueLB(); // expected-warning {{object backing the pointer will be dest}} - return StatusOr().valueLB(); // expected-warning {{returning address of local temporary}} + // The Owner doesn't own the object which its inner pointer points to. + std::string_view a = StatusOr().valueLB(); // OK + return StatusOr().valueLB(); // OK // No dangling diagnostics on non-lifetimebound methods. std::string_view b = StatusOr().valueNoLB(); @@ -652,7 +653,7 @@ Span test10(StatusOr> aa) { // Pointer> from Owner> Span test11(StatusOr> 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{}.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 +struct Foo { + const T& get() const [[clang::lifetimebound]]; + const T& getNoLB() const; +}; +std::string_view test2(Foo r1, Foo r2) { + std::string_view t1 = Foo().get(); // expected-warning {{object backing}} + t1 = Foo().get(); // expected-warning {{object backing}} + return r1.get(); // expected-warning {{address of stack}} + + std::string_view t2 = Foo().get(); + t2 = Foo().get(); + return r2.get(); + + // no warning on no-LB-annotated method. + std::string_view t3 = Foo().getNoLB(); + t3 = Foo().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 +struct MySpan { + MySpan(const std::vector& v); + using iterator = std::iterator; + iterator begin() const [[clang::lifetimebound]]; +}; +template +typename MySpan::iterator ReturnFirstIt(const MySpan& v [[clang::lifetimebound]]); + +void test4() { + std::vector v{1}; + // MySpan 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(v).begin(); + const int& t2 = *ReturnFirstIt(MySpan(v)); + // Ideally, we would diagnose the following case, but due to implementation + // constraints, we do not. + const int& t4 = *MySpan(std::vector{}).begin(); + + auto it1 = MySpan(v).begin(); // expected-warning {{temporary whose address is use}} + auto it2 = ReturnFirstIt(MySpan(v)); // expected-warning {{temporary whose address is used}} +} + +} // namespace LifetimeboundInterleave