Author: Haojian Wu Date: 2024-09-12T09:24:32+02:00 New Revision: 0683c4e839524c37fe4ddfa1bce1e31ba556041b
URL: https://github.com/llvm/llvm-project/commit/0683c4e839524c37fe4ddfa1bce1e31ba556041b DIFF: https://github.com/llvm/llvm-project/commit/0683c4e839524c37fe4ddfa1bce1e31ba556041b.diff LOG: Revert "[clang] Diagnose dangling issues for the "Container<GSLPointer>" case. (#107213)" This reverts commit e50131aa068f74daa70d4135c92020aadae3af33. It introduces a new false positive, see comment https://github.com/llvm/llvm-project/pull/107213#issuecomment-2345465256 Added: Modified: clang/docs/ReleaseNotes.rst clang/include/clang/Basic/AttrDocs.td clang/lib/Sema/CheckExprLifetime.cpp clang/test/Sema/warn-lifetime-analysis-nocfg.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 9860b25f2e7fa6..22749e96a7e3d3 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -301,8 +301,6 @@ Improvements to Clang's diagnostics - Clang now warns for u8 character literals used in C23 with ``-Wpre-c23-compat`` instead of ``-Wpre-c++17-compat``. -- Clang now diagnoses cases where a dangling ``GSLOwner<GSLPointer>`` object is constructed, e.g. ``std::vector<string_view> v = {std::string()};`` (#GH100526). - Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td index 9f72456d2da678..546e5100b79dd9 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -6690,20 +6690,6 @@ When the Owner's lifetime ends, it will consider the Pointer to be dangling. P.getInt(); // P is dangling } -If a template class is annotated with ``[[gsl::Owner]]``, and the first -instantiated template argument is a pointer type (raw pointer, or ``[[gsl::Pointer]]``), -the analysis will consider the instantiated class as a container of the pointer. -When constructing such an object from a GSL owner object, the analysis will -assume that the container holds a pointer to the owner object. Consequently, -when the owner object is destroyed, the pointer will be considered dangling. - -.. code-block:: c++ - - int f() { - std::vector<std::string_view> v = {std::string()}; // v holds a dangling pointer. - std::optional<std::string_view> o = std::string(); // o holds a dangling pointer. - } - }]; } diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp index 77c73f47658fe1..f62e18543851c1 100644 --- a/clang/lib/Sema/CheckExprLifetime.cpp +++ b/clang/lib/Sema/CheckExprLifetime.cpp @@ -267,26 +267,6 @@ static bool isInStlNamespace(const Decl *D) { return DC->isStdNamespace(); } -// Returns true if the given Record decl is a form of `GSLOwner<Pointer>` -// type, e.g. std::vector<string_view>, std::optional<string_view>. -static bool isContainerOfPointer(const RecordDecl *Container) { - if (const auto *CTSD = - dyn_cast_if_present<ClassTemplateSpecializationDecl>(Container)) { - if (!CTSD->hasAttr<OwnerAttr>()) // Container must be a GSL owner type. - return false; - const auto &TAs = CTSD->getTemplateArgs(); - return TAs.size() > 0 && TAs[0].getKind() == TemplateArgument::Type && - (isRecordWithAttr<PointerAttr>(TAs[0].getAsType()) || - TAs[0].getAsType()->isPointerType()); - } - return false; -} - -static bool isGSLOwner(QualType T) { - return isRecordWithAttr<OwnerAttr>(T) && - !isContainerOfPointer(T->getAsRecordDecl()); -} - static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee)) if (isRecordWithAttr<PointerAttr>(Conv->getConversionType())) @@ -295,7 +275,7 @@ static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) { return false; if (!isRecordWithAttr<PointerAttr>( Callee->getFunctionObjectParameterType()) && - !isGSLOwner(Callee->getFunctionObjectParameterType())) + !isRecordWithAttr<OwnerAttr>(Callee->getFunctionObjectParameterType())) return false; if (Callee->getReturnType()->isPointerType() || isRecordWithAttr<PointerAttr>(Callee->getReturnType())) { @@ -433,7 +413,7 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, // Once we initialized a value with a non gsl-owner reference, it can no // longer dangle. if (ReturnType->isReferenceType() && - !isGSLOwner(ReturnType->getPointeeType())) { + !isRecordWithAttr<OwnerAttr>(ReturnType->getPointeeType())) { for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) { if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit || PE.Kind == IndirectLocalPathEntry::LifetimeBoundCall) @@ -488,17 +468,12 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call, if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>()) VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]); else if (EnableGSLAnalysis && I == 0) { - // Perform GSL analysis for the first argument if (shouldTrackFirstArgument(Callee)) { VisitGSLPointerArg(Callee, Args[0]); - } else if (auto *Ctor = dyn_cast<CXXConstructExpr>(Call)) { - const auto *ClassD = Ctor->getConstructor()->getParent(); - // Two cases: - // a GSL pointer, e.g. std::string_view - // a container of GSL pointer, e.g. std::vector<string_view> - if (ClassD->hasAttr<PointerAttr>() || - (isContainerOfPointer(ClassD) && Callee->getNumParams() == 1)) - VisitGSLPointerArg(Ctor->getConstructor(), Args[0]); + } else if (auto *CCE = dyn_cast<CXXConstructExpr>(Call); + CCE && + CCE->getConstructor()->getParent()->hasAttr<PointerAttr>()) { + VisitGSLPointerArg(CCE->getConstructor(), Args[0]); } } } @@ -1010,12 +985,13 @@ static void checkExprLifetimeImpl(Sema &SemaRef, // int &p = *localUniquePtr; // someContainer.add(std::move(localUniquePtr)); // return p; - IsLocalGslOwner = isGSLOwner(L->getType()); + IsLocalGslOwner = isRecordWithAttr<OwnerAttr>(L->getType()); if (pathContainsInit(Path) || !IsLocalGslOwner) return false; } else { IsGslPtrValueFromGslTempOwner = - MTE && !MTE->getExtendingDecl() && isGSLOwner(MTE->getType()); + 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. diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp index 234e06f069074b..59357d0730a7d9 100644 --- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp +++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -158,30 +158,17 @@ auto begin(C &c) -> decltype(c.begin()); template<typename T, int N> T *begin(T (&array)[N]); -using size_t = decltype(sizeof(0)); - -template<typename T> -struct initializer_list { - const T* ptr; size_t sz; -}; template <typename T> struct vector { typedef __gnu_cxx::basic_iterator<T> iterator; iterator begin(); iterator end(); const T *data() const; - vector(); - vector(initializer_list<T> __l); - - template<typename InputIterator> - vector(InputIterator first, InputIterator __last); - T &at(int n); }; template<typename T> struct basic_string_view { - basic_string_view(); basic_string_view(const T *); const T *begin() const; }; @@ -216,21 +203,11 @@ template<typename T> struct optional { optional(); optional(const T&); - - template<typename U = T> - optional(U&& t); - - template<typename U> - optional(optional<U>&& __t); - T &operator*() &; T &&operator*() &&; T &value() &; T &&value() &&; }; -template<typename T> -optional<__decay(T)> make_optional(T&&); - template<typename T> struct stack { @@ -576,57 +553,3 @@ void test() { std::string_view svjkk1 = ReturnStringView(StrCat("bar", "x")); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}} } } // namespace GH100549 - -namespace GH100526 { -void test() { - std::vector<std::string_view> v1({std::string()}); // expected-warning {{object backing the pointer will be destroyed at the end}} - std::vector<std::string_view> v2({ - std::string(), // expected-warning {{object backing the pointer will be destroyed at the end}} - std::string_view() - }); - std::vector<std::string_view> v3({ - std::string_view(), - std::string() // expected-warning {{object backing the pointer will be destroyed at the end}} - }); - - std::optional<std::string_view> o1 = std::string(); // expected-warning {{object backing the pointer}} - - std::string s; - // This is a tricky use-after-free case, what it does: - // 1. make_optional creates a temporary "optional<string>"" object - // 2. the temporary object owns the underlying string which is copied from s. - // 3. the t3 object holds the view to the underlying string of the temporary object. - std::optional<std::string_view> o2 = std::make_optional(s); // expected-warning {{object backing the pointer}} - std::optional<std::string_view> o3 = std::optional<std::string>(s); // expected-warning {{object backing the pointer}} - std::optional<std::string_view> o4 = std::optional<std::string_view>(s); - - // FIXME: should work for assignment cases - v1 = {std::string()}; - o1 = std::string(); - - // no warning on copying pointers. - std::vector<std::string_view> n1 = {std::string_view()}; - std::optional<std::string_view> n2 = {std::string_view()}; - std::optional<std::string_view> n3 = std::string_view(); - std::optional<std::string_view> n4 = std::make_optional(std::string_view()); - const char* b = ""; - std::optional<std::string_view> n5 = std::make_optional(b); - std::optional<std::string_view> n6 = std::make_optional("test"); -} - -std::vector<std::string_view> test2(int i) { - std::vector<std::string_view> t; - if (i) - return t; // this is fine, no dangling - return std::vector<std::string_view>(t.begin(), t.end()); -} - -std::optional<std::string_view> test3(int i) { - std::string s; - std::string_view sv; - if (i) - return s; // expected-warning {{address of stack memory associated}} - return sv; // fine -} - -} // namespace GH100526 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
