[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
