Hi Richard, I'll move these behind a flag today. Moving forward, it would be great to have a way to dogfood those warnings without blocking you. We do run them over ~340 open source projects regularly, but clearly that is not enough :)
Thanks, Gabor On Fri, 23 Aug 2019 at 13:03, Richard Smith <rich...@metafoo.co.uk> wrote: > On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Hi Diana, >> hi Richard, >> >> thank you for the feedback! >> >> Diana, >> I remember that some gcc version had issues with raw string literals >> inside macros. I'll fix that to use normal >> string literals instead. >> >> Richard, >> We are definitely want the gsl::Pointer-based warnings that are enabled >> by default to be free of any false-positives. >> As you expected, this is showing a problem we have in another part of our >> analysis, and we will fix it before landing >> this PR again. >> > > It looks like this revert wasn't enough to unblock us. We're currently > unable to release compilers due to the scale of the new enabled-by-default > diagnostics produced by these warnings, and we're not happy about turning > off the existing (zero false positives) warning flags here in order to > unblock our releases, because they're so valuable in catching errors. I'd > expect others will hit similar issues when upgrading Clang. Even if there > were no false positives in the new warning, it appears to be undeployable > as-is because the new warning is behind the same warning flag as an > existing high-value warning. So I think we need the new warnings to be > moved behind different warning flags (a subgroup of ReturnStackAddress > would be OK, but it needs to be independently controllable). > > If this can be done imminently, that would be OK, but otherwise I think we > should temporarily roll this back until it can be moved to a separate > warning group. > > Both, sorry for the breakage! >> >> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith < >> rich...@metafoo.co.uk>: >> >>> Reverted in r369677. >>> >>> On Thu, 22 Aug 2019 at 10:34, Richard Smith <rich...@metafoo.co.uk> >>> wrote: >>> >>>> Hi Matthias, >>>> >>>> This introduces false positives into -Wreturn-stack-address for an >>>> example such as: >>>> >>>> #include <vector> >>>> >>>> std::vector<int>::iterator downcast_to(std::vector<int>::iterator >>>> value) { >>>> return *&value; >>>> } >>>> >>>> This breaks an internal build bot for us, so I'm going to revert this >>>> for now (though I expect this isn't the cause of the problem, but is rather >>>> exposing a problem elsewhere). >>>> >>>> If we can make the gsl::Pointer diagnostics false-positive-free, that's >>>> great, but otherwise we should use a different warning flag for the >>>> warnings that involve these annotations and use -Wreturn-stack-address for >>>> only the zero-false-positive cases that it historically diagnosed. >>>> >>>> Thanks, and sorry for the trouble. >>>> >>>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits < >>>> cfe-commits@lists.llvm.org> wrote: >>>> >>>>> Author: mgehre >>>>> Date: Wed Aug 21 15:08:59 2019 >>>>> New Revision: 369591 >>>>> >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=369591&view=rev >>>>> Log: >>>>> [LifetimeAnalysis] Support more STL idioms (template forward >>>>> declaration and DependentNameType) >>>>> >>>>> Summary: >>>>> This fixes inference of gsl::Pointer on std::set::iterator with >>>>> libstdc++ (the typedef for iterator >>>>> on the template is a DependentNameType - we can only put the >>>>> gsl::Pointer attribute >>>>> on the underlaying record after instantiation) >>>>> >>>>> inference of gsl::Pointer on std::vector::iterator with libc++ (the >>>>> class was forward-declared, >>>>> we added the gsl::Pointer on the canonical decl (the forward decl), >>>>> and later when the >>>>> template was instantiated, there was no attribute on the definition so >>>>> it was not instantiated). >>>>> >>>>> and a duplicate gsl::Pointer on some class with libstdc++ (we first >>>>> added an attribute to >>>>> a incomplete instantiation, and then another was copied from the >>>>> template definition >>>>> when the instantiation was completed). >>>>> >>>>> We now add the attributes to all redeclarations to fix thos issues and >>>>> make their usage easier. >>>>> >>>>> Reviewers: gribozavr >>>>> >>>>> Subscribers: Szelethus, xazax.hun, cfe-commits >>>>> >>>>> Tags: #clang >>>>> >>>>> Differential Revision: https://reviews.llvm.org/D66179 >>>>> >>>>> Added: >>>>> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp >>>>> Modified: >>>>> cfe/trunk/lib/Sema/SemaAttr.cpp >>>>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp >>>>> cfe/trunk/lib/Sema/SemaInit.cpp >>>>> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp >>>>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp >>>>> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp >>>>> cfe/trunk/unittests/Sema/CMakeLists.txt >>>>> >>>>> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Sema/SemaAttr.cpp (original) >>>>> +++ cfe/trunk/lib/Sema/SemaAttr.cpp Wed Aug 21 15:08:59 2019 >>>>> @@ -88,13 +88,11 @@ void Sema::AddMsStructLayoutForRecord(Re >>>>> template <typename Attribute> >>>>> static void addGslOwnerPointerAttributeIfNotExisting(ASTContext >>>>> &Context, >>>>> CXXRecordDecl >>>>> *Record) { >>>>> - CXXRecordDecl *Canonical = Record->getCanonicalDecl(); >>>>> - if (Canonical->hasAttr<OwnerAttr>() || >>>>> Canonical->hasAttr<PointerAttr>()) >>>>> + if (Record->hasAttr<OwnerAttr>() || Record->hasAttr<PointerAttr>()) >>>>> return; >>>>> >>>>> - Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context, >>>>> - /*DerefType*/ nullptr, >>>>> - /*Spelling=*/0)); >>>>> + for (Decl *Redecl : Record->redecls()) >>>>> + Redecl->addAttr(Attribute::CreateImplicit(Context, >>>>> /*DerefType=*/nullptr)); >>>>> } >>>>> >>>>> void Sema::inferGslPointerAttribute(NamedDecl *ND, >>>>> @@ -189,8 +187,7 @@ void Sema::inferGslOwnerPointerAttribute >>>>> >>>>> // Handle classes that directly appear in std namespace. >>>>> if (Record->isInStdNamespace()) { >>>>> - CXXRecordDecl *Canonical = Record->getCanonicalDecl(); >>>>> - if (Canonical->hasAttr<OwnerAttr>() || >>>>> Canonical->hasAttr<PointerAttr>()) >>>>> + if (Record->hasAttr<OwnerAttr>() || >>>>> Record->hasAttr<PointerAttr>()) >>>>> return; >>>>> >>>>> if (StdOwners.count(Record->getName())) >>>>> >>>>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original) >>>>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Wed Aug 21 15:08:59 2019 >>>>> @@ -4592,9 +4592,11 @@ static void handleLifetimeCategoryAttr(S >>>>> } >>>>> return; >>>>> } >>>>> - D->addAttr(::new (S.Context) >>>>> - OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc, >>>>> - AL.getAttributeSpellingListIndex())); >>>>> + for (Decl *Redecl : D->redecls()) { >>>>> + Redecl->addAttr(::new (S.Context) >>>>> + OwnerAttr(AL.getRange(), S.Context, >>>>> DerefTypeLoc, >>>>> + >>>>> AL.getAttributeSpellingListIndex())); >>>>> + } >>>>> } else { >>>>> if (checkAttrMutualExclusion<OwnerAttr>(S, D, AL)) >>>>> return; >>>>> @@ -4609,9 +4611,11 @@ static void handleLifetimeCategoryAttr(S >>>>> } >>>>> return; >>>>> } >>>>> - D->addAttr(::new (S.Context) >>>>> - PointerAttr(AL.getRange(), S.Context, DerefTypeLoc, >>>>> - AL.getAttributeSpellingListIndex())); >>>>> + for (Decl *Redecl : D->redecls()) { >>>>> + Redecl->addAttr(::new (S.Context) >>>>> + PointerAttr(AL.getRange(), S.Context, >>>>> DerefTypeLoc, >>>>> + >>>>> AL.getAttributeSpellingListIndex())); >>>>> + } >>>>> } >>>>> } >>>>> >>>>> >>>>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original) >>>>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Wed Aug 21 15:08:59 2019 >>>>> @@ -6561,7 +6561,7 @@ static void visitLocalsRetainedByReferen >>>>> >>>>> template <typename T> static bool isRecordWithAttr(QualType Type) { >>>>> if (auto *RD = Type->getAsCXXRecordDecl()) >>>>> - return RD->getCanonicalDecl()->hasAttr<T>(); >>>>> + return RD->hasAttr<T>(); >>>>> return false; >>>>> } >>>>> >>>>> @@ -6672,7 +6672,7 @@ static void handleGslAnnotatedTypes(Indi >>>>> >>>>> if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) { >>>>> const auto *Ctor = CCE->getConstructor(); >>>>> - const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl(); >>>>> + const CXXRecordDecl *RD = Ctor->getParent(); >>>>> if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>()) >>>>> VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]); >>>>> } >>>>> >>>>> Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp (original) >>>>> +++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp Wed Aug 21 >>>>> 15:08:59 2019 >>>>> @@ -552,6 +552,18 @@ void Sema::InstantiateAttrs(const MultiL >>>>> continue; >>>>> } >>>>> >>>>> + if (auto *A = dyn_cast<PointerAttr>(TmplAttr)) { >>>>> + if (!New->hasAttr<PointerAttr>()) >>>>> + New->addAttr(A->clone(Context)); >>>>> + continue; >>>>> + } >>>>> + >>>>> + if (auto *A = dyn_cast<OwnerAttr>(TmplAttr)) { >>>>> + if (!New->hasAttr<OwnerAttr>()) >>>>> + New->addAttr(A->clone(Context)); >>>>> + continue; >>>>> + } >>>>> + >>>>> assert(!TmplAttr->isPackExpansion()); >>>>> if (TmplAttr->isLateParsed() && LateAttrs) { >>>>> // Late parsed attributes must be instantiated and attached >>>>> after the >>>>> @@ -711,6 +723,9 @@ Decl *TemplateDeclInstantiator::Instanti >>>>> >>>>> SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef); >>>>> >>>>> + if (D->getUnderlyingType()->getAs<DependentNameType>()) >>>>> + SemaRef.inferGslPointerAttribute(Typedef); >>>>> + >>>>> Typedef->setAccess(D->getAccess()); >>>>> >>>>> return Typedef; >>>>> >>>>> Modified: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp (original) >>>>> +++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp Wed Aug 21 >>>>> 15:08:59 2019 >>>>> @@ -92,6 +92,59 @@ public: >>>>> static_assert(sizeof(unordered_map<int>::iterator), ""); // Force >>>>> instantiation. >>>>> } // namespace inlinens >>>>> >>>>> +// The iterator typedef is a DependentNameType. >>>>> +template <typename T> >>>>> +class __unordered_multimap_iterator {}; >>>>> +// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator >>>>> +// CHECK: ClassTemplateSpecializationDecl {{.*}} >>>>> __unordered_multimap_iterator >>>>> +// CHECK: TemplateArgument type 'int' >>>>> +// CHECK: PointerAttr >>>>> + >>>>> +template <typename T> >>>>> +class __unordered_multimap_base { >>>>> +public: >>>>> + using iterator = __unordered_multimap_iterator<T>; >>>>> +}; >>>>> + >>>>> +template <typename T> >>>>> +class unordered_multimap { >>>>> + // CHECK: ClassTemplateDecl {{.*}} unordered_multimap >>>>> + // CHECK: OwnerAttr {{.*}} >>>>> + // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap >>>>> + // CHECK: OwnerAttr {{.*}} >>>>> +public: >>>>> + using _Mybase = __unordered_multimap_base<T>; >>>>> + using iterator = typename _Mybase::iterator; >>>>> +}; >>>>> +static_assert(sizeof(unordered_multimap<int>::iterator), ""); // >>>>> Force instantiation. >>>>> + >>>>> +// The canonical declaration of the iterator template is not its >>>>> definition. >>>>> +template <typename T> >>>>> +class __unordered_multiset_iterator; >>>>> +// CHECK: ClassTemplateDecl {{.*}} __unordered_multiset_iterator >>>>> +// CHECK: PointerAttr >>>>> +// CHECK: ClassTemplateSpecializationDecl {{.*}} >>>>> __unordered_multiset_iterator >>>>> +// CHECK: TemplateArgument type 'int' >>>>> +// CHECK: PointerAttr >>>>> + >>>>> +template <typename T> >>>>> +class __unordered_multiset_iterator { >>>>> + // CHECK: ClassTemplateDecl {{.*}} prev {{.*}} >>>>> __unordered_multiset_iterator >>>>> + // CHECK: PointerAttr >>>>> +}; >>>>> + >>>>> +template <typename T> >>>>> +class unordered_multiset { >>>>> + // CHECK: ClassTemplateDecl {{.*}} unordered_multiset >>>>> + // CHECK: OwnerAttr {{.*}} >>>>> + // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multiset >>>>> + // CHECK: OwnerAttr {{.*}} >>>>> +public: >>>>> + using iterator = __unordered_multiset_iterator<T>; >>>>> +}; >>>>> + >>>>> +static_assert(sizeof(unordered_multiset<int>::iterator), ""); // >>>>> Force instantiation. >>>>> + >>>>> // std::list has an implicit gsl::Owner attribute, >>>>> // but explicit attributes take precedence. >>>>> template <typename T> >>>>> >>>>> Modified: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp (original) >>>>> +++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp Wed Aug 21 >>>>> 15:08:59 2019 >>>>> @@ -105,3 +105,20 @@ class [[gsl::Owner(int)]] AddTheSameLate >>>>> class [[gsl::Owner(int)]] AddTheSameLater; >>>>> // CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater >>>>> // CHECK: OwnerAttr {{.*}} int >>>>> + >>>>> +template <class T> >>>>> +class [[gsl::Owner]] ForwardDeclared; >>>>> +// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared >>>>> +// CHECK: OwnerAttr {{.*}} >>>>> +// CHECK: ClassTemplateSpecializationDecl {{.*}} ForwardDeclared >>>>> +// CHECK: TemplateArgument type 'int' >>>>> +// CHECK: OwnerAttr {{.*}} >>>>> + >>>>> +template <class T> >>>>> +class [[gsl::Owner]] ForwardDeclared { >>>>> +// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared >>>>> +// CHECK: CXXRecordDecl {{.*}} ForwardDeclared definition >>>>> +// CHECK: OwnerAttr {{.*}} >>>>> +}; >>>>> + >>>>> +static_assert(sizeof(ForwardDeclared<int>), ""); // Force >>>>> instantiation. >>>>> >>>>> Modified: cfe/trunk/unittests/Sema/CMakeLists.txt >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/CMakeLists.txt?rev=369591&r1=369590&r2=369591&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/unittests/Sema/CMakeLists.txt (original) >>>>> +++ cfe/trunk/unittests/Sema/CMakeLists.txt Wed Aug 21 15:08:59 2019 >>>>> @@ -5,11 +5,13 @@ set(LLVM_LINK_COMPONENTS >>>>> add_clang_unittest(SemaTests >>>>> ExternalSemaSourceTest.cpp >>>>> CodeCompleteTest.cpp >>>>> + GslOwnerPointerInference.cpp >>>>> ) >>>>> >>>>> clang_target_link_libraries(SemaTests >>>>> PRIVATE >>>>> clangAST >>>>> + clangASTMatchers >>>>> clangBasic >>>>> clangFrontend >>>>> clangParse >>>>> >>>>> Added: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp?rev=369591&view=auto >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp (added) >>>>> +++ cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp Wed Aug 21 >>>>> 15:08:59 2019 >>>>> @@ -0,0 +1,61 @@ >>>>> +//== unittests/Sema/GslOwnerPointerInference.cpp - gsl::Owner/Pointer >>>>> ========// >>>>> +// >>>>> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM >>>>> Exceptions. >>>>> +// See https://llvm.org/LICENSE.txt for license information. >>>>> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception >>>>> +// >>>>> >>>>> +//===----------------------------------------------------------------------===// >>>>> + >>>>> +#include "../ASTMatchers/ASTMatchersTest.h" >>>>> +#include "clang/ASTMatchers/ASTMatchers.h" >>>>> +#include "gtest/gtest.h" >>>>> + >>>>> +namespace clang { >>>>> +using namespace ast_matchers; >>>>> + >>>>> +TEST(OwnerPointer, BothHaveAttributes) { >>>>> + EXPECT_TRUE(matches( >>>>> + R"cpp( >>>>> + template<class T> >>>>> + class [[gsl::Owner]] C; >>>>> + >>>>> + template<class T> >>>>> + class [[gsl::Owner]] C {}; >>>>> + >>>>> + C<int> c; >>>>> + )cpp", >>>>> + classTemplateSpecializationDecl(hasName("C"), >>>>> hasAttr(clang::attr::Owner)))); >>>>> +} >>>>> + >>>>> +TEST(OwnerPointer, ForwardDeclOnly) { >>>>> + EXPECT_TRUE(matches( >>>>> + R"cpp( >>>>> + template<class T> >>>>> + class [[gsl::Owner]] C; >>>>> + >>>>> + template<class T> >>>>> + class C {}; >>>>> + >>>>> + C<int> c; >>>>> + )cpp", >>>>> + classTemplateSpecializationDecl(hasName("C"), >>>>> hasAttr(clang::attr::Owner)))); >>>>> +} >>>>> + >>>>> +TEST(OwnerPointer, LateForwardDeclOnly) { >>>>> + EXPECT_TRUE(matches( >>>>> + R"cpp( >>>>> + template<class T> >>>>> + class C; >>>>> + >>>>> + template<class T> >>>>> + class C {}; >>>>> + >>>>> + template<class T> >>>>> + class [[gsl::Owner]] C; >>>>> + >>>>> + C<int> c; >>>>> + )cpp", >>>>> + classTemplateSpecializationDecl(hasName("C"), >>>>> hasAttr(clang::attr::Owner)))); >>>>> +} >>>>> + >>>>> +} // namespace clang >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> cfe-commits@lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>> >>>> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits