r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

Nico Weber via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 23 16:05:50 PDT 2019 

On Thu, Aug 22, 2019 at 4:05 PM Matthias Gehre via cfe-commits <
cfe-commits at 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.
>

I think it's only a problem if the raw string is in a macro. Instead of

  FOO(R"(asdf)");

you can do

  const char kStr[] = R"(asdf)";
  FOO(kStr);

and gcc should be happy. (In case raw strings buy you something over normal
string literals.)


>
> 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.
>
> Both, sorry for the breakage!
>
> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
> richard at metafoo.co.uk>:
>
>> Reverted in r369677.
>>
>> On Thu, 22 Aug 2019 at 10:34, Richard Smith <richard at 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 at 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 at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190823/95b86081/attachment-0001.html>

More information about the cfe-commits mailing list