[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre created this revision. mgehre added a reviewer: gribozavr. Herald added a subscriber: Szelethus. Herald added a project: clang. 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). Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66179 Files: clang/lib/Sema/SemaAttr.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp === --- clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp +++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp @@ -92,6 +92,59 @@ static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation. } // namespace inlinens +// The iterator typedef is a DependentNameType. +template +class __unordered_multimap_iterator {}; +// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator +// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multimap_iterator +// CHECK: TemplateArgument type 'int' +// CHECK: PointerAttr + +template +class __unordered_multimap_base { +public: + using iterator = __unordered_multimap_iterator; +}; + +template +class unordered_multimap { + // CHECK: ClassTemplateDecl {{.*}} unordered_multimap + // CHECK: OwnerAttr {{.*}} + // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap + // CHECK: OwnerAttr {{.*}} +public: + using _Mybase = __unordered_multimap_base; + using iterator = typename _Mybase::iterator; +}; +static_assert(sizeof(unordered_multimap::iterator), ""); // Force instantiation. + +// The canonical declaration of the iterator template is not its definition. +template +class __unordered_multiset_iterator; +// CHECK: ClassTemplateDecl {{.*}} __unordered_multiset_iterator +// CHECK: PointerAttr +// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multiset_iterator +// CHECK: TemplateArgument type 'int' +// CHECK: PointerAttr + +template +class __unordered_multiset_iterator { + // CHECK: ClassTemplateDecl {{.*}} prev {{.*}} __unordered_multiset_iterator + // CHECK: PointerAttr +}; + +template +class unordered_multiset { + // CHECK: ClassTemplateDecl {{.*}} unordered_multiset + // CHECK: OwnerAttr {{.*}} + // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multiset + // CHECK: OwnerAttr {{.*}} +public: + using iterator = __unordered_multiset_iterator; +}; + +static_assert(sizeof(unordered_multiset::iterator), ""); // Force instantiation. + // std::list has an implicit gsl::Owner attribute, // but explicit attributes take precedence. template Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp === --- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -552,6 +552,18 @@ continue; } +if (auto *A = dyn_cast(TmplAttr)) { + if (!New->hasAttr()) +New->addAttr(A->clone(Context)); + continue; +} + +if (auto *A = dyn_cast(TmplAttr)) { + if (!New->hasAttr()) +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 @@ SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef); + if (D->getUnderlyingType()->getAs()) +SemaRef.inferGslPointerAttribute(Typedef); + Typedef->setAccess(D->getAccess()); return Typedef; Index: clang/lib/Sema/SemaAttr.cpp === --- clang/lib/Sema/SemaAttr.cpp +++ clang/lib/Sema/SemaAttr.cpp @@ -88,13 +88,20 @@ template static void addGslOwnerPointerAttributeIfNotExisting(ASTContext &Context, CXXRecordDecl *Record) { - CXXRecordDecl *Canonical = Record->getCanonicalDecl(); - if (Canonical->hasAttr() || Canonical->hasAttr()) + if (Record->hasAttr() || Record->hasAttr()) return; - Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context, - /*DerefType*/ nullptr, - /*Spelling=*/0)); + Record->addAttr(::new (Context)
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:102 + dyn_cast_or_null(Record->getDescribedTemplate())) { +if (auto *Def = Record->getDefinition()) + addGslOwnerPointerAttributeIfNotExisting(Context, Def); I wonder why this is necessary. Sema should call inference on every CXXRecordDecl. Is it because of incorrect short-circuiting in `inferGslPointerAttribute` that I'm pointing out below? Comment at: clang/lib/Sema/SemaAttr.cpp:143 +addGslOwnerPointerAttributeIfNotExisting( +Context, UnderlyingRecord->getCanonicalDecl()); } So... what's the contract for pointer and owner attributes, are they expected to be present on every declaration in the redeclaration chain, or is only one sufficient? Seems like here we're adding the attribute only to the canonical decl of the iterator, which will lead to the same sort of issue that this patch is trying to fix. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; Should this code not do this short-circuit check? It is prone to going out of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, like it did just now. If you want to check for attribute before doing the string search, you could pass the string set into `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it should infer the attribute or not. Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95 +// The iterator typedef is a DependentNameType. +template This test file is getting pretty long and subtle, with lots of things are being mixed into one file without isolation. WDYT about refactoring it into a unit test that uses AST matchers to verify attribute presence? See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples. Each test case would look approximately like this: ``` EXPECT_TRUE(matches( "template class ...", classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer))); ``` Each test case would be isolated from other tests, each test would have a name (and optionally a comment) that will make it obvious what exactly is being tested. It would be also possible to verify things like "The iterator typedef is a DependentNameType" to ensure that we're testing exactly what we want to test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked 6 inline comments as done. mgehre added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:102 + dyn_cast_or_null(Record->getDescribedTemplate())) { +if (auto *Def = Record->getDefinition()) + addGslOwnerPointerAttributeIfNotExisting(Context, Def); gribozavr wrote: > I wonder why this is necessary. Sema should call inference on every > CXXRecordDecl. Is it because of incorrect short-circuiting in > `inferGslPointerAttribute` that I'm pointing out below? The contract is: A CXXRecordDecl is a Pointer/Owner if and only if its canonical declaration has the Pointer/Owner attribute. The analysis only checks this on concrete classes (excuse me if I'm using the wrong term here), i.e. non-template classes or instantiation of template classes. During the inference, we might also put the attributes onto the templated declaration of a ClassTemplateDecl. The Pointer/Owner attributes here will not be used by the analysis. We just put them there so clang will copy them to each instantiation, where they will be used by the analysis. From what I understand, clang copies the attributes of the definition of the templated declaration to the instantiation. In addition, when clang sees a redeclaration (e.g. of a template), it will also copy the attributes from the previous/canonical (?) declaration. In the problematic case ``` // The canonical declaration of the iterator template is not its definition. template class __unordered_multiset_iterator; template class __unordered_multiset_iterator { }; template class unordered_multiset { public: using iterator = __unordered_multiset_iterator; }; static_assert(sizeof(unordered_multiset::iterator), ""); // Force instantiation. ``` clang would (before this PR): 1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is the canonical declaration, but not a definition) 2. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is not the canonical declaration, but its definition) 3. While processing the `using iterator` line, add a PointerAttr to the canonical declaration (from point 1) 4. While instantiating `__unordered_multiset_iterator`, copy attributes from the template definition (from point 2), which are none Result: The `ClassTemplateSpecializationDecl` for `__unordered_multiset_iterator` would not have the `PointerAttr`. On the other hand, a swapped example ``` template class __unordered_multiset_iterator; template class unordered_multiset { public: using iterator = __unordered_multiset_iterator; }; template class __unordered_multiset_iterator { }; static_assert(sizeof(unordered_multiset::iterator), ""); // Force instantiation. ``` would work because clang would (before this PR): 1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is the canonical declaration, but not a definition) 2. While processing the `using iterator` line, add a PointerAttr to the canonical declaration (from point 1) 3. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for `__unordered_multiset_iterator` (this is not the canonical declaration, but its definition); copy all attributes from a previous declaration, i.e. the PointerAttr from point 1 4. While instantiating `__unordered_multiset_iterator`, copy PointerAttr from the template definition (from point 3) So we cannot just put the PointerAttr on the definition of the class template because there might be none yet (like in the second example). We need to put it on the definition in case it was already parsed. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; gribozavr wrote: > Should this code not do this short-circuit check? It is prone to going out of > sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, like it did > just now. > > If you want to check for attribute before doing the string search, you could > pass the string set into `addGslOwnerPointerAttributeIfNotExisting`, and let > that decide if it should infer the attribute or not. Yes, the `hasAttr` check here is an optimization to avoid the string search. I don't think I can move the string search into `addGslOwnerPointerAttributeIfNotExisting`, because that function is called from other call sites that don't care about a set of names. Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95 +// The iterator typedef is a DependentNameType. +template gribozavr wrote: > This test file is getting pretty long and subtle, with lots of things are > being mixed into one file without isolation. > > WDYT about refactoring it into a unit test that uses AST matchers to verify > attribute presence? > > See clang/unittests/ASTM
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; mgehre wrote: > gribozavr wrote: > > Should this code not do this short-circuit check? It is prone to going out > > of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, like > > it did just now. > > > > If you want to check for attribute before doing the string search, you > > could pass the string set into `addGslOwnerPointerAttributeIfNotExisting`, > > and let that decide if it should infer the attribute or not. > Yes, the `hasAttr` check here is an optimization to avoid the string search. > I don't think I can move the string search into > `addGslOwnerPointerAttributeIfNotExisting`, because that function is called > from other call sites that don't care about a set of names. Sorry I wasn't clear enough -- the issue that I noticed is that this check looks at the canonical decl, while `addGslOwnerPointerAttributeIfNotExisting` attaches the attribute to the decl itself -- that's what I meant by "out of sync". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked 4 inline comments as done. mgehre added a comment. I now start to wonder if we should instead put the attribute on all declarations (instead of just the canonical). Later redeclarations automatically inherit the attributes. This would make both the checking easier (just check any declaration, no need to obtain the canonical first), and remove the special case for adding to the definition of templated record for ClassTemplateDecls. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; gribozavr wrote: > mgehre wrote: > > gribozavr wrote: > > > Should this code not do this short-circuit check? It is prone to going > > > out of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, > > > like it did just now. > > > > > > If you want to check for attribute before doing the string search, you > > > could pass the string set into > > > `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it > > > should infer the attribute or not. > > Yes, the `hasAttr` check here is an optimization to avoid the string > > search. I don't think I can move the string search into > > `addGslOwnerPointerAttributeIfNotExisting`, because that function is called > > from other call sites that don't care about a set of names. > Sorry I wasn't clear enough -- the issue that I noticed is that this check > looks at the canonical decl, while `addGslOwnerPointerAttributeIfNotExisting` > attaches the attribute to the decl itself -- that's what I meant by "out of > sync". I make sure that `addGslOwnerPointerAttributeIfNotExisting` is only called with the canonical decl as argument, which the caller usually has around. The only exception to this is when addGslOwnerPointerAttributeIfNotExisting calls itself to attach an attribute to the definition of templated class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
gribozavr added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:200 CXXRecordDecl *Canonical = Record->getCanonicalDecl(); if (Canonical->hasAttr() || Canonical->hasAttr()) return; mgehre wrote: > gribozavr wrote: > > mgehre wrote: > > > gribozavr wrote: > > > > Should this code not do this short-circuit check? It is prone to going > > > > out of sync with the code in > > > > `addGslOwnerPointerAttributeIfNotExisting`, like it did just now. > > > > > > > > If you want to check for attribute before doing the string search, you > > > > could pass the string set into > > > > `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it > > > > should infer the attribute or not. > > > Yes, the `hasAttr` check here is an optimization to avoid the string > > > search. I don't think I can move the string search into > > > `addGslOwnerPointerAttributeIfNotExisting`, because that function is > > > called > > > from other call sites that don't care about a set of names. > > Sorry I wasn't clear enough -- the issue that I noticed is that this check > > looks at the canonical decl, while > > `addGslOwnerPointerAttributeIfNotExisting` attaches the attribute to the > > decl itself -- that's what I meant by "out of sync". > I make sure that `addGslOwnerPointerAttributeIfNotExisting` is only called > with the canonical decl as argument, which the caller usually has around. The > only exception to this is when addGslOwnerPointerAttributeIfNotExisting calls > itself to attach an attribute to the definition of templated class. I see. This is a tricky contract, and ".getCanonicalDecl()" at callsites feel somewhat random. I think it should be at least documented. However... > I now start to wonder if we should instead put the attribute on all > declarations (instead of just the canonical). Later redeclarations > automatically inherit the attributes. > This would make both the checking easier (just check any declaration, no need > to obtain the canonical first), and remove the special > case for adding to the definition of templated record for ClassTemplateDecls. I'd support that. Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95 +// The iterator typedef is a DependentNameType. +template mgehre wrote: > gribozavr wrote: > > This test file is getting pretty long and subtle, with lots of things are > > being mixed into one file without isolation. > > > > WDYT about refactoring it into a unit test that uses AST matchers to verify > > attribute presence? > > > > See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples. > > > > Each test case would look approximately like this: > > > > ``` > > EXPECT_TRUE(matches( > > "template class ...", > > classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer))); > > ``` > > > > Each test case would be isolated from other tests, each test would have a > > name (and optionally a comment) that will make it obvious what exactly is > > being tested. > > > > It would be also possible to verify things like "The iterator typedef is a > > DependentNameType" to ensure that we're testing exactly what we want to > > test. > I like the AST matcher approach! This test file is really hard to debug - I > usually copy a test to its own file for debugging. > Would you be okay with deferring the testing change to the next PR? Of course! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:94 - Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context, - /*DerefType*/ nullptr, - /*Spelling=*/0)); + Record->addAttr(::new (Context) Attribute(SourceRange{}, Context, +/*DerefType*/ nullptr, Doesn't the attribute have a `CreateImplicit` static method? If so, we could use that :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre added a comment. I now add the attributes to all redeclarations to make the logic a bit easier to follow. Comment at: clang/lib/Sema/SemaAttr.cpp:94 - Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context, - /*DerefType*/ nullptr, - /*Spelling=*/0)); + Record->addAttr(::new (Context) Attribute(SourceRange{}, Context, +/*DerefType*/ nullptr, xazax.hun wrote: > Doesn't the attribute have a `CreateImplicit` static method? If so, we could > use that :) Nice, didn't know that! Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95 +// The iterator typedef is a DependentNameType. +template gribozavr wrote: > mgehre wrote: > > gribozavr wrote: > > > This test file is getting pretty long and subtle, with lots of things are > > > being mixed into one file without isolation. > > > > > > WDYT about refactoring it into a unit test that uses AST matchers to > > > verify attribute presence? > > > > > > See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples. > > > > > > Each test case would look approximately like this: > > > > > > ``` > > > EXPECT_TRUE(matches( > > > "template class ...", > > > classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer))); > > > ``` > > > > > > Each test case would be isolated from other tests, each test would have a > > > name (and optionally a comment) that will make it obvious what exactly is > > > being tested. > > > > > > It would be also possible to verify things like "The iterator typedef is > > > a DependentNameType" to ensure that we're testing exactly what we want to > > > test. > > I like the AST matcher approach! This test file is really hard to debug - I > > usually copy a test to its own file for debugging. > > Would you be okay with deferring the testing change to the next PR? > Of course! Thanks! I added the some tests for this particular change to a new unittest and will migrate the rest of the tests later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre updated this revision to Diff 216247. mgehre marked 6 inline comments as done. mgehre added a comment. Herald added a subscriber: mgorny. - Put OwnerAttr/PointerAttr on all redeclarations - clang/lib/Sema/SemaAttr.cpp: Use Attribute::CreateImplicit as requested in review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 Files: clang/lib/Sema/SemaAttr.cpp clang/lib/Sema/SemaDeclAttr.cpp clang/lib/Sema/SemaInit.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp clang/test/SemaCXX/attr-gsl-owner-pointer.cpp clang/unittests/Sema/CMakeLists.txt clang/unittests/Sema/GslOwnerPointerInference.cpp Index: clang/unittests/Sema/GslOwnerPointerInference.cpp === --- /dev/null +++ clang/unittests/Sema/GslOwnerPointerInference.cpp @@ -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 [[gsl::Owner]] C; + + template + class [[gsl::Owner]] C {}; + + C c; + )cpp", +classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner; +} + +TEST(OwnerPointer, ForwardDeclOnly) { + EXPECT_TRUE(matches( +R"cpp( + template + class [[gsl::Owner]] C; + + template + class C {}; + + C c; + )cpp", +classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner; +} + +TEST(OwnerPointer, LateForwardDeclOnly) { + EXPECT_TRUE(matches( +R"cpp( + template + class C; + + template + class C {}; + + template + class [[gsl::Owner]] C; + + C c; + )cpp", +classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner; +} + +} // namespace clang Index: clang/unittests/Sema/CMakeLists.txt === --- clang/unittests/Sema/CMakeLists.txt +++ clang/unittests/Sema/CMakeLists.txt @@ -5,11 +5,13 @@ add_clang_unittest(SemaTests ExternalSemaSourceTest.cpp CodeCompleteTest.cpp + GslOwnerPointerInference.cpp ) clang_target_link_libraries(SemaTests PRIVATE clangAST + clangASTMatchers clangBasic clangFrontend clangParse Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp === --- clang/test/SemaCXX/attr-gsl-owner-pointer.cpp +++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp @@ -105,3 +105,20 @@ class [[gsl::Owner(int)]] AddTheSameLater; // CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater // CHECK: OwnerAttr {{.*}} int + +template +class [[gsl::Owner]] ForwardDeclared; +// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared +// CHECK: OwnerAttr {{.*}} +// CHECK: ClassTemplateSpecializationDecl {{.*}} ForwardDeclared +// CHECK: TemplateArgument type 'int' +// CHECK: OwnerAttr {{.*}} + +template +class [[gsl::Owner]] ForwardDeclared { +// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared +// CHECK: CXXRecordDecl {{.*}} ForwardDeclared definition +// CHECK: OwnerAttr {{.*}} +}; + +static_assert(sizeof(ForwardDeclared), ""); // Force instantiation. Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp === --- clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp +++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp @@ -92,6 +92,59 @@ static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation. } // namespace inlinens +// The iterator typedef is a DependentNameType. +template +class __unordered_multimap_iterator {}; +// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator +// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multimap_iterator +// CHECK: TemplateArgument type 'int' +// CHECK: PointerAttr + +template +class __unordered_multimap_base { +public: + using iterator = __unordered_multimap_iterator; +}; + +template +class unordered_multimap { + // CHECK: ClassTemplateDecl {{.*}} unordered_multimap + // CHECK: OwnerAttr {{.*}} + // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap + // CHECK: OwnerAttr {{.*}} +public: + using _Mybase = __unordered_multimap_base; + using iterator = typename _Mybase::iterator; +}; +static_assert(sizeof(unordered_multimap::iterator), ""); // Force instantiation. + +// The canonical decla
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4596 +for (Decl *Redecl : D->redecls()) { + Redecl->addAttr(::new (S.Context) + OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc, Add it only if it does not already exist? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6627 const auto *Ctor = CCE->getConstructor(); const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl(); if (CCE->getNumArgs() > 0 && RD->hasAttr()) You can also remove the `getCanonicalDecl` call here :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked 3 inline comments as done. mgehre added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4596 +for (Decl *Redecl : D->redecls()) { + Redecl->addAttr(::new (S.Context) + OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc, gribozavr wrote: > Add it only if it does not already exist? If a previous decl had that Attribute, then we would have inherited it, and the check above for an existing attribute would have led to an early return. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
This revision was automatically updated to reflect the committed changes. mgehre marked an inline comment as done. Closed by commit rL369591: [LifetimeAnalysis] Support more STL idioms (template forward declaration and… (authored by mgehre, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D66179?vs=216247&id=216498#toc Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 Files: 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 cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp Index: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp === --- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -552,6 +552,18 @@ continue; } +if (auto *A = dyn_cast(TmplAttr)) { + if (!New->hasAttr()) +New->addAttr(A->clone(Context)); + continue; +} + +if (auto *A = dyn_cast(TmplAttr)) { + if (!New->hasAttr()) +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 @@ SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef); + if (D->getUnderlyingType()->getAs()) +SemaRef.inferGslPointerAttribute(Typedef); + Typedef->setAccess(D->getAccess()); return Typedef; Index: cfe/trunk/lib/Sema/SemaInit.cpp === --- cfe/trunk/lib/Sema/SemaInit.cpp +++ cfe/trunk/lib/Sema/SemaInit.cpp @@ -6561,7 +6561,7 @@ template static bool isRecordWithAttr(QualType Type) { if (auto *RD = Type->getAsCXXRecordDecl()) -return RD->getCanonicalDecl()->hasAttr(); +return RD->hasAttr(); return false; } @@ -6672,7 +6672,7 @@ if (auto *CCE = dyn_cast(Call)) { const auto *Ctor = CCE->getConstructor(); -const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl(); +const CXXRecordDecl *RD = Ctor->getParent(); if (CCE->getNumArgs() > 0 && RD->hasAttr()) VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]); } Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp === --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp @@ -4592,9 +4592,11 @@ } 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(S, D, AL)) return; @@ -4609,9 +4611,11 @@ } 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())); +} } } Index: cfe/trunk/lib/Sema/SemaAttr.cpp === --- cfe/trunk/lib/Sema/SemaAttr.cpp +++ cfe/trunk/lib/Sema/SemaAttr.cpp @@ -88,13 +88,11 @@ template static void addGslOwnerPointerAttributeIfNotExisting(ASTContext &Context, CXXRecordDecl *Record) { - CXXRecordDecl *Canonical = Record->getCanonicalDecl(); - if (Canonical->hasAttr() || Canonical->hasAttr()) + if (Record->hasAttr() || Record->hasAttr()) 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 @@ // Handle classes that directly appear in std namespace. if (Record->isInStdNamespace()) { -CXXRecordDecl *Canonical = Record->getCanonicalDecl(); -if (Canonical->hasAttr() || Canonical->hasAttr()) +if (
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
thakis added a comment. Similar to Comment at: cfe/trunk/unittests/Sema/CMakeLists.txt:14 clangAST + clangASTMatchers clangBasic Is it necessary to use ASTMachers to test this? It'd be good if SemaTests wouldn't have to depend on ASTMatchers (for linking speed, as well for layering hygiene). Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" This weird relative include path is a hint that this isn't the intended use :) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked an inline comment as done. mgehre added inline comments. Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" thakis wrote: > This weird relative include path is a hint that this isn't the intended use :) True. But ... I thought about copying the `matches` function from STMatchersTest.h, but together with all callees, that's many lines of code. Alternatively, I could implement my own AST walking and property checking, but then the test would be much less readable. As yet another alternative, I though about moving the `GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`, but that would also be strange because it actually tests Sema functionality. What would be your suggestion? (It's good that we discuss this now because I'm planning to extend this unit test further). Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
thakis added inline comments. Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" mgehre wrote: > thakis wrote: > > This weird relative include path is a hint that this isn't the intended use > > :) > True. But ... > I thought about copying the `matches` function from STMatchersTest.h, but > together with all callees, that's many lines of code. > Alternatively, I could implement my own AST walking and property checking, > but then the test would be much less readable. > As yet another alternative, I though about moving the > `GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`, > but that would also be strange because it actually tests Sema functionality. > > What would be your suggestion? (It's good that we discuss this now because > I'm planning to extend this unit test further). Do you need the full matches() function? With the current test, it's not even clear to me what exactly it tests since it looks very integration-testy and not very unit-testy. Maybe if you make the test narrower, you don't that much scaffolding? (Integration-type tests are supposed to be lit tests, and unit tests are supposed to test small details that are difficult to test via lit.) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
thakis added a comment. And, unrelatedly, for libc++, shouldn't we just add the attribute to the libc++ source instead of trying to infer it in the compiler? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked 2 inline comments as done. mgehre added inline comments. Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" thakis wrote: > mgehre wrote: > > thakis wrote: > > > This weird relative include path is a hint that this isn't the intended > > > use :) > > True. But ... > > I thought about copying the `matches` function from STMatchersTest.h, but > > together with all callees, that's many lines of code. > > Alternatively, I could implement my own AST walking and property checking, > > but then the test would be much less readable. > > As yet another alternative, I though about moving the > > `GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`, > > but that would also be strange because it actually tests Sema functionality. > > > > What would be your suggestion? (It's good that we discuss this now because > > I'm planning to extend this unit test further). > Do you need the full matches() function? With the current test, it's not even > clear to me what exactly it tests since it looks very integration-testy and > not very unit-testy. Maybe if you make the test narrower, you don't that much > scaffolding? (Integration-type tests are supposed to be lit tests, and unit > tests are supposed to test small details that are difficult to test via lit.) The purpose of the test is to verify that the ClassTemplateSpecialization gets the Owner attribute, independent of which of the redeclarations of the template had the attribute attached. The second thing that I want to test here in the future is that the inference for std:: types leads to correct Owner/Pointer attributes on the `ClassTemplateSpecialization`., i.e. a type named `std::vector` should have OwnerAttr on each of its `ClassTemplateSpecialization`. We currently do this in a lit test here https://reviews.llvm.org/differential/changeset/?ref=1574879 via checking the AST dump, but that is really hard to debug in cases of failures, and not entirely clear that it checks exactly what we want. (E.g. we can check that a `ClassTemplateSpecializationDecl` line is followed by a `PointerAttr` line, but we don't actually know if that attribute hierarchically belongs to that decl or a another one). So @gribozavr suggested to me to instead break this integration tests into smaller chunks using the ASTMatchers, which at least solves that problem. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)
mgehre marked an inline comment as done. mgehre added a comment. Yes, for libc++ we could add the annotations to its source code, and eventually this would be the cleaner solution. Currently, that is neither sufficient (because one could use libstdc++, MSVC or an older libc++ version) nor necessary (the inference we need for libstdc++ / MSVC also works for libc++), so it's currently low on our priority list. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits