gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[mgehre]

Summary

Make the DerefType, i.e. the argument of gsl::Owner/gsl::Pointer optional for now.

The DerefType is used when infering lifetime annotations of functions, e.g.
how a return value can relate to the arguments. Our initial checks should
work without that kind of inference and instead use explicit annotations,
so we don't need the DerefType right now.
And having it optional makes the implicit/default annotations for std:: types
easier to implement. I think the DerefType is also optional in the current MSVC
implementation.

This seems to be the first attribute that has an optional Type argument,
so I needed to fix two things in tablegen:

• Don't crash AST dump when Owner/Pointer has no Type argument
• Don't crash when pretty-printing an Attribute with optional type argument

Add is_gsl_owner/is_gsl_pointer builtins. They are very useful to test
if the annotations have been attached correctly.

Hard code gsl::Owner/gsl::Pointer for std types. The paper mentions
some types explicitly. Generally, all containers and their iterators are
covered. For iterators, we cover both the case that they are defined
as an nested class or as an typedef/using. I have started to test this
implementation against some real standard library implementations, namely
libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

The tests are currently here

https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.sh
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp

I think due to their dependency on a standard library, they are not a good fit
for clang/test/. Where else could I put them?

[gribozavr]

The tests are currently here
I think due to their dependency on a standard library, they are not a good fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and check them into clang/test.

[xazax.hun]

In D64448#1577109, @gribozavr wrote:

The tests are currently here
I think due to their dependency on a standard library, they are not a good fit for clang/test/. Where else could I put them?

Make some mocks that reproduce the salient parts of different libraries, the coding patterns they use, and check them into clang/test.

Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp. I was thinking about adding the corresponding tests to libc++ as well so we have better chances to get some notifications if a new language feature/implementation trick is introduced.

[gribozavr]

Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp.

I see. Sorry, but that seems insufficient to me -- different libraries use different patterns. For example, libc++ wraps everything in std in an inline namespace. I don't know how various versions of libstdc++ differ.

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I'd suggest to split type trait implementations into a separate patch.

Are these type traits being exposed only for testing? I'm not sure it is a good idea to do that -- people will end up using them in production code -- is that a concern? If so, maybe it would be better to test through warnings.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

This test and related code changes could be split off into a separate patch.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86 ↗

(On Diff #208813)

I feel like these tests would be better off in a separate file.

It would be also good to explain which exact library we are trying to imitate in this test. Different libraries use different coding patterns.

103 ↗

(On Diff #208813)

Is it actually defined like that?

140 ↗

(On Diff #208813)

Unclear what this test is testing.

If there's something special about thread (e.g., it looks very much like an owner or a pointer, and a buggy implementation can easily declare thread to be an owner or a pointer), please explain that in a comment.

If you're testing that some random name is not being picked up by inference rules, I'd suggest to use an obviously-fictional name ("class type_unknown_to_compiler;")

[JonasToth]

Just as Sidenote in case you were not aware: There is a check in clang-tidy for gsl::owner (https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html)
It will benefit from your annotation and can be simplified.

[mgehre]

In D64448#1577779, @gribozavr wrote:

For example, libc++ wraps everything in std in an inline namespace.

I believed that I had written a test for inline namespaces, but seems that I shouldn't be working in the middle of the night.
I will add one.

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I copied that approach from https://reviews.llvm.org/D50119.
If people do it in production code, then at least the two leading underscores should tell them "I'm not supposed to use this".

I considered a test through warnings, but how would I trigger them? Add -Wlifetime-categories-debug which emits notes whenever a [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class?

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

I was thinking whether it made sense to separate

• fixing the AST dump of attributes with optional type parameter when there is not such attribute
• introduce and attribute with optional type parameter while AST dumping it is broken

so I decided that both are closely related. Otherwise the fix and its test are in separate PRs?

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86 ↗

(On Diff #208813)

This is imitating techniques from different libraries - all techniques that this implementation supports.

To check if all techniques that this implementation supports are enough for real standard library implementations,
I use https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp against them. Various versions of libstdc++
and libc++ passed. I will test MSVC standard library next. If they would use a technique that this implementation does not support yet,
I will add support for that and the corresponding test here.
I might fix MSVC support (if needed) only in the following PR:

103 ↗

(On Diff #208813)

There might be a standard library implementation that does it like this. This tests that we will use the using iterator = set_iterator<T>; in class set { to attach the [[gsl::Pointer]] to the set_iterator.

140 ↗

(On Diff #208813)

Agreed, I will rename this to an obviously-fictional name.

[gribozavr]

In D64448#1581719, @mgehre wrote:

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

Yes, I saw the testcase -- but how do different libstdc++ versions differ?

[gribozavr]

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

If people do it in production code, then at least the two leading underscores should tell them "I'm not supposed to use this".

That's not what two underscores mean. These custom type traits, being language extensions, must have a name that won't collide with any user-defined name, hence two underscores. Two underscores mean nothing about whether the user is allowed to use it or not. Sure the code might become non-portable to other compilers, but that's not what the concern is. My concern is that code that people write might become unportable to future versions of Clang, where we would have to change behavior of these type traits (or it would just subtly change in some corner case and we won't notice since we don't consider it a public API).

I considered a test through warnings, but how would I trigger them?

I meant regular warnings, that are the objective of this analysis -- warnings about dereferencing dangling pointers. If we get those warnings for the types-under-test, then obviously they have the correct annotations from the compiler's point of view.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

Totally makes sense to have the fix and the test is the same PR, but they seem to be separable from attribute inference for std types, right?

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86 ↗

(On Diff #208813)

This is imitating techniques from different libraries - all techniques that this implementation supports.

Okay -- please add comments about each technique then (and ideally, which libraries use them). Right now (for me, who didn't write the patch), the test looks like it is testing inference for a bunch of types, not for a bunch of techniques -- the differences are subtle and non-obvious.

103 ↗

(On Diff #208813)

Then std::set_iterator must have an implementation-reserved name, like std::__set_iterator.

[mgehre]

• Implement comments

[mgehre]

In D64448#1581771, @gribozavr wrote:

In D64448#1581719, @mgehre wrote:

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

Yes, I saw the testcase -- but how do different libstdc++ versions differ?

I didn't know whether they would differ, but the test tells me that they don't differ significantly (i.e. in introducing new techniques).
Would you like me to test with other standard libraries (besides MSVC, which I already planned)?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I spent a lot of time debugging on the full lifetime analysis, and knowing exactly which type has which Attribute (especially if inference is involved) was quite important.
I would say that adding additional code to trigger a real lifetime warning

• requires that we first add some lifetime warnings to clang (currently in a different PR)
• obscures the purpose of the check, i.e. checking the attributes and not the warnings themselves
• and makes debugging hard when the test fails (warnings involve at least one Owner and one Pointer, so either of those could have made the test fail)

I'd prefer if we can test the attributes directly.

clang/test/AST/ast-dump-attr.cpp

222 ↗

(On Diff #208813)

Yes, you are right. And Aaron would like to have the type parameter already optional in D63954, so I will move this part over there.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86 ↗

(On Diff #208813)

Sure, I will add comments.

103 ↗

(On Diff #208813)

True, I can call it like that to make the test more realistic.

[gribozavr]

In D64448#1581866, @mgehre wrote:

I didn't know whether they would differ, but the test tells me that they don't differ significantly (i.e. in introducing new techniques).

Okay -- I would prefer if you intentionally looked at the code before declaring a library as supported, or asked someone who knows about the differences, but it is your call.

Would you like me to test with other standard libraries (besides MSVC, which I already planned)?

It is your call which libraries you would like to support.

Another question -- in the latest iteration of the patch the inference code disappeared -- was it intentional?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

I agree that being able to test these properties definitely simplifies testing. I am worried about adding language extension though, and would like someone like Richard Smith to LGTM this approach.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

86 ↗

(On Diff #209370)

s/builtin attributes/Test attribute inference for types in the standard library./

88 ↗

(On Diff #209370)

s/added to/inferred for/

93 ↗

(On Diff #209370)

Sorry, but what do you mean by a "concrete template"?

"Attributes are inferred on class template instantiations."

102 ↗

(On Diff #209370)

lowercase "attributes"

114 ↗

(On Diff #209370)

lowercase "attributes"

115 ↗

(On Diff #209370)

Could you separate the inline namespace test from the iterator part into another test, say, for unordered_map?

[rsmith]

clang/include/clang/Basic/Attr.td

2773 ↗

(On Diff #208813)

On the surface this doesn't appear to make sense, but I think that's a pre-existing bug.

@aaron.ballman Is this field name reversed from the intent? I think what it means is "meaningful on (non-defining) class declaration", which is... the opposite of what its name suggests. Looking at the tablegen implementation, it appears that setting this to 1 causes the attribute to be instantiated when instantiating a class declaration, not only when instantiating a class definition.

clang/include/clang/Basic/AttrDocs.td

4167–4168 ↗

(On Diff #208813)

... and what does the attribute mean when the argument is omitted?

4180 ↗

(On Diff #208813)

Similarly, what happens in that case?

clang/include/clang/Basic/TokenKinds.def

486 ↗

(On Diff #208813)

If you just want this for testing, could you instead use an -ast-dump test and see if the attributes were added? Alternatively I'd be OK having these as just-for-testing traits if you give them names that makes their status as being for-testing, unsupported, may-be-removed-or-changed-at-any-time clear. (Eg, __clang_debug_is_gsl_owner.)

Generally we would like the __is_* type traits to exactly match the semantics of the corresponding standard C++ std::is_* type traits (except where we're forced to do something else for compatibility).

[mgehre]

• Merge branch 'lifetime-categories' into hardcode-lifetime-categories; Split test; Move test to AST dump; fix for comments;

[mgehre]

This should fix all open comments.

clang/include/clang/Basic/AttrDocs.td

4167–4168 ↗

(On Diff #208813)

I added text that it is currently ignored. We will use the argument in future analysis, but already add parsing of it for forward compatibility.
Per Aaron's request, this part moved to the parent PR, D63954.

clang/test/SemaCXX/attr-gsl-owner-pointer.cpp

93 ↗

(On Diff #209370)

It says "complete", and I mean "not forward declared".

86 ↗

(On Diff #208813)

I moved the std tests to a different file.

[lebedev.ri]

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

[mgehre]

• Remove type traits

[mgehre]

In D64448#1595017, @lebedev.ri wrote:

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

Thanks for the question! The implicitly added attributes have an invalid source location.

[lebedev.ri]

In D64448#1595022, @mgehre wrote:

In D64448#1595017, @lebedev.ri wrote:

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

Thanks for the question! The implicitly added attributes have an invalid source location.

Sounds good, thanks.

[gribozavr]

clang/include/clang/Sema/Sema.h

6097 ↗

(On Diff #211010)

It seems like this function does not add gsl::Owner.

clang/lib/Sema/SemaTemplate.cpp

1689 ↗

(On Diff #211010)

It shouldn't be necessary to perform inference here, instead, the attributes should be instantiated, see instantiateDependentAlignedAttr in SemaTemplateInstantiateDecl.cpp for an example.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

702 ↗

(On Diff #211010)

Ditto, should not be necessary to perform inference here.

[mgehre]

• Fix comments
• Add Pointer via typedef on ClassTemplateDecl

clang/include/clang/Sema/Sema.h

6097 ↗

(On Diff #211010)

Fixed comment

clang/lib/Sema/SemaTemplate.cpp

1689 ↗

(On Diff #211010)

From what I understand, here the attribute is put on the ClassTemplateDecl. Without this, there would be no attribute to instantiate from?
The instantiation of OwnerAttr/PointerAttr is auto-generated into instantiateTemplateAttribute() in build/tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

702 ↗

(On Diff #211010)

I'll move this to already act on the typedef in the ClassTemplateDecl (instead of the instantiation here).

[mgehre]

• Add missing check

[mgehre]

• Fix crash

[gribozavr]

clang/include/clang/Sema/Sema.h

6122 ↗

(On Diff #212446)

s/addDefault/infer/ ? (everywhere in the patch)

cfe/trunk/include/clang/Basic/AttrDocs.td

.. code-block:: c++
 
   class [[gsl::Owner(int)]] IntOwner {
   private:
     int value;
   public:
     int *getInt() { return &value; }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
 See Pointer_ for an example.
 }];
 }
 
 def LifetimePointerDocs : Documentation {
.. code-block:: c++
 
   class [[gsl::Pointer(int)]] IntPointer {
   private:
     int *valuePointer;
   public:
     int *getInt() { return &valuePointer; }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
 Example:
 When constructing an instance of a class annotated like this (a Pointer) from
 an instance of a class annotated with ``[[gsl::Owner]]`` (an Owner),
 then the analysis will consider the Pointer to point inside the Owner.
 When the Owner's lifetime ends, it will consider the Pointer to be dangling.

cfe/trunk/include/clang/Sema/Sema.h

 
   void referenceDLLExportedClassMethods();
 
   void propagateDLLAttrToBaseClassTemplate(
       CXXRecordDecl *Class, Attr *ClassAttr,
       ClassTemplateSpecializationDecl *BaseTemplateSpec,
       SourceLocation BaseLoc);
 
+  /// Add gsl::Pointer attribute to std::container::iterator
+  /// \param ND The declaration that introduces the name
+  /// std::container::iterator. \param UnderlyingRecord The record named by ND.
+  void inferGslPointerAttribute(NamedDecl *ND, CXXRecordDecl *UnderlyingRecord);
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void inferGslOwnerPointerAttribute(CXXRecordDecl *Record);
+
+  /// Add [[gsl::Pointer]] attributes for std:: types.
+  void inferGslPointerAttribute(TypedefNameDecl *TD);
+
   void CheckCompletedCXXClass(CXXRecordDecl *Record);
 
   /// Check that the C++ class annoated with "trivial_abi" satisfies all the
   /// conditions that are needed for the attribute to have an effect.
   void checkIllFormedTrivialABIStruct(CXXRecordDecl &RD);
 
   void ActOnFinishCXXMemberSpecification(Scope *S, SourceLocation RLoc,
                                          Decl *TagDecl, SourceLocation LBrac,

cfe/trunk/lib/Sema/SemaAttr.cpp

void Sema::AddMsStructLayoutForRecord(RecordDecl *RD) {
   // FIXME: We should merge AddAlignmentAttributesForRecord with
   // AddMsStructLayoutForRecord into AddPragmaAttributesForRecord, which takes
   // all active pragmas and applies them as attributes to class definitions.
   if (VtorDispStack.CurrentValue != getLangOpts().VtorDispMode)
     RD->addAttr(
         MSVtorDispAttr::CreateImplicit(Context, VtorDispStack.CurrentValue));
 }
 
+template <typename Attribute>
+static void addGslOwnerPointerAttributeIfNotExisting(ASTContext &Context,
+                                                     CXXRecordDecl *Record) {
+  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
+  if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
+    return;
+
+  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
+                                               /*DerefType*/ nullptr,
+                                               /*Spelling=*/0));
+}
+
+void Sema::inferGslPointerAttribute(NamedDecl *ND,
+                                    CXXRecordDecl *UnderlyingRecord) {
+  if (!UnderlyingRecord)
+    return;
+
+  const auto *Parent = dyn_cast<CXXRecordDecl>(ND->getDeclContext());
+  if (!Parent)
+    return;
+
+  static llvm::StringSet<> Containers{
+      "array",
+      "basic_string",
+      "deque",
+      "forward_list",
+      "vector",
+      "list",
+      "map",
+      "multiset",
+      "multimap",
+      "priority_queue",
+      "queue",
+      "set",
+      "stack",
+      "unordered_set",
+      "unordered_map",
+      "unordered_multiset",
+      "unordered_multimap",
+  };
+
+  static llvm::StringSet<> Iterators{"iterator", "const_iterator",
+                                     "reverse_iterator",
+                                     "const_reverse_iterator"};
+
+  if (Parent->isInStdNamespace() && Iterators.count(ND->getName()) &&
+      Containers.count(Parent->getName()))
+    addGslOwnerPointerAttributeIfNotExisting<PointerAttr>(Context,
+                                                          UnderlyingRecord);
+}
+
+void Sema::inferGslPointerAttribute(TypedefNameDecl *TD) {
+
+  QualType Canonical = TD->getUnderlyingType().getCanonicalType();
+
+  CXXRecordDecl *RD = Canonical->getAsCXXRecordDecl();
+  if (!RD) {
+    if (auto *TST =
+            dyn_cast<TemplateSpecializationType>(Canonical.getTypePtr())) {
+
+      RD = dyn_cast_or_null<CXXRecordDecl>(
+          TST->getTemplateName().getAsTemplateDecl()->getTemplatedDecl());
+    }
+  }
+
+  inferGslPointerAttribute(TD, RD);
+}
+
+void Sema::inferGslOwnerPointerAttribute(CXXRecordDecl *Record) {
+  static llvm::StringSet<> StdOwners{
+      "any",
+      "array",
+      "basic_regex",
+      "basic_string",
+      "deque",
+      "forward_list",
+      "vector",
+      "list",
+      "map",
+      "multiset",
+      "multimap",
+      "optional",
+      "priority_queue",
+      "queue",
+      "set",
+      "stack",
+      "unique_ptr",
+      "unordered_set",
+      "unordered_map",
+      "unordered_multiset",
+      "unordered_multimap",
+      "variant",
+  };
+  static llvm::StringSet<> StdPointers{
+      "basic_string_view",
+      "reference_wrapper",
+      "regex_iterator",
+  };
+
+  if (!Record->getIdentifier())
+    return;
+
+  // Handle classes that directly appear in std namespace.
+  if (Record->isInStdNamespace()) {
+    CXXRecordDecl *Canonical = Record->getCanonicalDecl();
+    if (Canonical->hasAttr<OwnerAttr>() || Canonical->hasAttr<PointerAttr>())
+      return;
+
+    if (StdOwners.count(Record->getName()))
+      addGslOwnerPointerAttributeIfNotExisting<OwnerAttr>(Context, Record);
+    else if (StdPointers.count(Record->getName()))
+      addGslOwnerPointerAttributeIfNotExisting<PointerAttr>(Context, Record);
+
+    return;
+  }
+
+  // Handle nested classes that could be a gsl::Pointer.
+  inferGslPointerAttribute(Record, Record);
+}
+
 void Sema::ActOnPragmaOptionsAlign(PragmaOptionsAlignKind Kind,
                                    SourceLocation PragmaLoc) {
   PragmaMsStackAction Action = Sema::PSK_Reset;
   unsigned Alignment = 0;
   switch (Kind) {
     // For all targets we support native and natural are the same.
     //
     // FIXME: This is not true on Darwin/PPC.

cfe/trunk/lib/Sema/SemaDecl.cpp

Sema::ActOnTypedefNameDecl(Scope *S, DeclContext *DC, TypedefNameDecl *NewTD,
   // Merge the decl with the existing one if appropriate. If the decl is
   // in an outer scope, it isn't the same thing.
   FilterLookupForScope(Previous, DC, S, /*ConsiderLinkage*/false,
                        /*AllowInlineNamespace*/false);
   filterNonConflictingPreviousTypedefDecls(*this, NewTD, Previous);
   if (!Previous.empty()) {
     Redeclaration = true;
     MergeTypedefNameDecl(S, NewTD, Previous);
+  } else {
+    inferGslPointerAttribute(NewTD);
   }
 
   if (ShadowedDecl && !Redeclaration)
     CheckShadow(NewTD, ShadowedDecl, Previous);
 
   // If this is the C FILE type, notify the AST context.
   if (IdentifierInfo *II = NewTD->getIdentifier())
     if (!NewTD->isInvalidDecl() &&
if (IdentifierInfo *II = New->getIdentifier())
     if (!New->isInvalidDecl() &&
         New->getDeclContext()->getRedeclContext()->isTranslationUnit() &&
         II->isStr("FILE"))
       Context.setFILEDecl(New);
 
   if (PrevDecl)
     mergeDeclAttributes(New, PrevDecl);
 
+  if (auto *CXXRD = dyn_cast<CXXRecordDecl>(New))
+    inferGslOwnerPointerAttribute(CXXRD);
+
   // If there's a #pragma GCC visibility in scope, set the visibility of this
   // record.
   AddPushedVisibilityAttribute(New);
 
   if (isMemberSpecialization && !New->isInvalidDecl())
     CompleteMemberSpecialization(New, Previous);
 
   OwnedDecl = true;

cfe/trunk/lib/Sema/SemaTemplate.cpp

if (TUK == TUK_Definition && (!SkipBody || !SkipBody->ShouldSkip))
     NewClass->startDefinition();
 
   ProcessDeclAttributeList(S, NewClass, Attr);
 
   if (PrevClassTemplate)
     mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   AddPushedVisibilityAttribute(NewClass);
+  inferGslOwnerPointerAttribute(NewClass);
 
   if (TUK != TUK_Friend) {
     // Per C++ [basic.scope.temp]p2, skip the template parameter scopes.
     Scope *Outer = S;
     while ((Outer->getFlags() & Scope::TemplateParamScope) != 0)
       Outer = Outer->getParent();
     PushOnScopeChains(NewTemplate, Outer);
   } else {

cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template <typename T>
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector<int>), "");           // Force instantiation.
+static_assert(sizeof(vector<int>::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template <typename T>
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template <typename T>
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator<T>;
+};
+static_assert(sizeof(set<int>::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template <typename T>
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template <typename T>
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator<T> iterator;
+};
+static_assert(sizeof(map<int>::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template <typename T>
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template <typename T>
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator<T> iterator;
+};
+static_assert(sizeof(unordered_map<int>::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template <typename T>
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list<int>), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+    class CharT,
+    class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template <class T>
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user