Add lifetime categories attributes

[mgehre]

Summary

Joint work with @xazax.hun.

This is the first part of work announced in
"[RFC] Adding lifetime analysis to clang" [0],
i.e. the addition of the [[gsl::Owner(T)]] and
[[gsl::Pointer(T)]] attributes, which
will enable user-defined types to participate in
the lifetime analysis (which will be part of the
next PR).
The type T here is called "DerefType" in the paper,
and denotes the type that an Owner owns and a Pointer
points to. E.g. std::vector<int> should be annotated
with [[gsl::Owner(int)]] and
a std::vector<int>::iterator with [[gsl::Pointer(int)]].

We explicitly allow to add an annotation after
the definition of the class to allow adding annotations
to external source from by the user, e.g.

#include <vector>

namespace std {
template<typename T, typename Alloc>
class [[gsl::Owner(T)]] vector;
}

until we can ship a standard library with annotations.

[0] http://lists.llvm.org/pipermail/cfe-dev/2018-November/060355.html

[mgehre]

Fix typo

[erik.pilkington]

We explicitly allow to add an annotation after
the definition of the class to allow adding annotations
to external source from by the user, e.g.

#include <vector>

namespace std {
template<typename T, typename Alloc>
class [[gsl::Owner(T)]] vector;
}

Wait, does that even work? What if the vector was declared in an inline namespace in the header? Seems like a strange recommendation for users. Is there some reason you can't just add these annotations to standard libraries? I doubt libcxx would have a problem with getting better warnings on their types.

clang/lib/Sema/SemaDeclAttr.cpp

4560–4561 ↗

(On Diff #207138)

This is duplicated with the first line in the function.

4564 ↗

(On Diff #207138)

diag::warn_duplicate_attr?

[mgehre]

We can try to add the annotations to libcxx (which will still take time),
but there are also other standard libraries (libstdc++, MSVC) that
can be used with clang.
Eventually everybody will have lifetime annotations (of course ;-) ),
but in the meantime there seems to be no other way to enable
the analysis on unmodified STL.

My idea would be to maintain a lifetime-annotations headers (outside of clang repo?)
with #ifdef magic to add lifetime annotations for major STL variants/versions as a drop-in
to enable lifetime analysis. It's not nice, but only temporary and at least better than requiring
to change your STL to get lifetime analysis.

[xazax.hun]

I actually think we should hard code STL (and maybe some other popular) types into the compiler. I.e.: if an STL type is unannotated and the warnings are turned on, we could look up the default annotations from a table and append them during parsing. This could be done in a separate patch.

[mgehre]

We can and should explore both options and see what works best.

[mgehre]

Address some review comments.

[mgehre]

clang/lib/Sema/SemaDeclAttr.cpp

4560–4561 ↗

(On Diff #207138)

Removed the first line, which only checked for duplicate attributes on the same declaration, but we must check (here) that all matching
declaration are consistent - by performing all checks on the canonical declaration.

[aaron.ballman]

In D63954#1562928, @erik.pilkington wrote:

We explicitly allow to add an annotation after
the definition of the class to allow adding annotations
to external source from by the user, e.g.

#include <vector>

namespace std {
template<typename T, typename Alloc>
class [[gsl::Owner(T)]] vector;
}

Wait, does that even work? What if the vector was declared in an inline namespace in the header? Seems like a strange recommendation for users. Is there some reason you can't just add these annotations to standard libraries? I doubt libcxx would have a problem with getting better warnings on their types.

We do not want to encourage users to redeclare things within namespace std like this -- I believe that is UB.

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

Has Microsoft already added support for this? We had an unfortunate problem with gsl::suppress where we introduced the attribute first and MSVC introduced it under a different, incompatible syntax. I would strongly like to avoid repeating that.

clang/include/clang/Basic/AttrDocs.td

4167 ↗

(On Diff #207235)

the owning object's lifetime end -> the owning object's lifetime ends

4177 ↗

(On Diff #207235)

You don't reference P in the rest of the docs, is that intentional?

4186 ↗

(On Diff #207235)

Please keep the newline at the end of the file.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

4537 ↗

(On Diff #207235)

Under what circumstances is this check needed? I believe this is already automatically handled for you.

4546–4547 ↗

(On Diff #207235)

Is void really the only problematic type? What about incomplete types, for instance?

4553 ↗

(On Diff #207235)

Will this work? What happens if we see a forward declaration with this attribute first and then see the canonical declaration later? I suspect this work needs to be done when merging declaration attributes instead.

4554 ↗

(On Diff #207235)

Formatting.

4557 ↗

(On Diff #207235)

Please use a name other than Attr (as that's a type name).

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

61 ↗

(On Diff #207235)

Please add a newline to the end of the file.

[gribozavr]

We explicitly allow to add an annotation after the definition of the class to allow adding annotations to external source from by the user

Asking users to forward-declare anything from the standard library is a very bad idea -- in fact it is undefined behavior. https://stackoverflow.com/questions/307343/forward-declare-an-stl-container

The compiler should just know about standard library types and attach attributes automatically.

[xazax.hun]

In D63954#1564448, @gribozavr wrote:

We explicitly allow to add an annotation after the definition of the class to allow adding annotations to external source from by the user

Asking users to forward-declare anything from the standard library is a very bad idea -- in fact it is undefined behavior. https://stackoverflow.com/questions/307343/forward-declare-an-stl-container

The compiler should just know about standard library types and attach attributes automatically.

I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g. -wlifetime is added to the compiler invocation?

On the other hand I still think it is useful to give the users the option to maintain a header with forward declarations to add the annotations to other (non-standard) 3rd party types. These headers might be fragile to 3rd party changes but could still be a better option than maintaining patches on top of 3rd party libraries. Having API notes upstream would be a superior solution here and I might look into upstreaming it at some point, but I think this is something that could be addressed later on. What do you think?

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

I will double check this. Her Sutter's paper have this spelling and I believe that the Microsoft implementation is following the same spelling. The main difference is that the MSVC version does not have a type argument at this point but they do plan to add it as an optional argument. (They want to infer the pointee type when it is not provided.) The clang community do not want the inference, hence we made the type argument required. Always providing the type argument should be compatible with future versions of the MSVC implementation.

clang/lib/Sema/SemaDeclAttr.cpp

4537 ↗

(On Diff #207235)

It is handled automatically indeed, thanks!

4546–4547 ↗

(On Diff #207235)

I think incomplete types are fine. The only information we actually need here is the name the pointee types. E.g. for a SomeOwner<Incomplete> which is annotated Owner(Incomplete), we will assume that a method returning Incomplete*, std::reference_wrapper<Incomplete> etc will return an object pointing to the memory owned by SomeOwner.

4553 ↗

(On Diff #207235)

For TagDecls the canonical decl is the first declaration:

TagDecl *TagDecl::getCanonicalDecl() { return getFirstDecl(); }

So I suspect we can never see a non-canonical declaration first. And once we see the canonical declaration it remains the same no matter how many new declaration do we see.

4564 ↗

(On Diff #207138)

We actually allow duplicated attributes as long as they are not conflicting. The reason why we introduced a new diagnostic is that the source of the problem is the conflict, i.e.: the two attributes are contradicting each other. Not the duplication.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

4553 ↗

(On Diff #207235)

This is the scenario I am worried about:

struct [[whatever]] Foo;
struct Foo {};

[xazax.hun]

clang/lib/Sema/SemaDeclAttr.cpp

4553 ↗

(On Diff #207235)

This works well as the forward declaration is the canonical declaration since it is the first declaration in the redecl chain. A declaration being canonical or being a definition are two independent properties.

The only caveat here is that the users of these attributes always need to query the attribute of the canonical decl.

[gribozavr]

I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g. -wlifetime is added to the compiler invocation?

Warning flags should not change the AST -- compiler engineers don't expect that. So if Clang is going to perform inference, it should either always do it, or it should be guarded by an -f flag, not a -W flag.

On the other hand I still think it is useful to give the users the option to maintain a header with forward declarations to add the annotations to other (non-standard) 3rd party types. These headers might be fragile to 3rd party changes but could still be a better option than maintaining patches on top of 3rd party libraries. Having API notes upstream would be a superior solution here and I might look into upstreaming it at some point, but I think this is something that could be addressed later on. What do you think?

I think it is acceptable for 3rd party libraries, but there's already a solution for 3rd party libraries -- API notes in Swift's fork of Clang. It has been successfully used by Apple for 5 years for this exact purpose (adding attributes to existing libraries without changing headers), and only needs to be upstreamed to Clang.

[xazax.hun]

In D63954#1565109, @gribozavr wrote:

I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g. -wlifetime is added to the compiler invocation?

Warning flags should not change the AST -- compiler engineers don't expect that. So if Clang is going to perform inference, it should either always do it, or it should be guarded by an -f flag, not a -W flag.

Thanks, this make sense. My only concern would be that a -W flag without the "inference" for STL types would be useless. Is it ok to make the driver add a -f flag automatically if a warning is turned on or would you find that confusing as well?

On the other hand I still think it is useful to give the users the option to maintain a header with forward declarations to add the annotations to other (non-standard) 3rd party types. These headers might be fragile to 3rd party changes but could still be a better option than maintaining patches on top of 3rd party libraries. Having API notes upstream would be a superior solution here and I might look into upstreaming it at some point, but I think this is something that could be addressed later on. What do you think?

I think it is acceptable for 3rd party libraries, but there's already a solution for 3rd party libraries -- API notes in Swift's fork of Clang. It has been successfully used by Apple for 5 years for this exact purpose (adding attributes to existing libraries without changing headers), and only needs to be upstreamed to Clang.

Supporting the forward declarations way is only a few lines of code now, supporting API Notes is a much larger effort. I would also prefer the latter but I think having this work not blocked on upstreaming API notes is essential to get this upstreamed. Or would you prefer not supporting the 3rd party library use-case until API Notes are upstreamed?

[mgehre]

• Address more review comments.
• git-clang-format

[gribozavr]

In D63954#1565128, @xazax.hun wrote:

In D63954#1565109, @gribozavr wrote:

I agree. In a follow-up patch we will add the attributes to STL types during parsing. Do you think it is OK to always add the attributes or should we only do it if a flag, e.g. -wlifetime is added to the compiler invocation?

Warning flags should not change the AST -- compiler engineers don't expect that. So if Clang is going to perform inference, it should either always do it, or it should be guarded by an -f flag, not a -W flag.

Thanks, this make sense. My only concern would be that a -W flag without the "inference" for STL types would be useless. Is it ok to make the driver add a -f flag automatically if a warning is turned on or would you find that confusing as well?

Why not always attach attributes to standard library types? I only suggested the -f flag in case it is necessary to gate this functionality for some reason (e.g., performance).

On the other hand I still think it is useful to give the users the option to maintain a header with forward declarations to add the annotations to other (non-standard) 3rd party types. These headers might be fragile to 3rd party changes but could still be a better option than maintaining patches on top of 3rd party libraries. Having API notes upstream would be a superior solution here and I might look into upstreaming it at some point, but I think this is something that could be addressed later on. What do you think?

I think it is acceptable for 3rd party libraries, but there's already a solution for 3rd party libraries -- API notes in Swift's fork of Clang. It has been successfully used by Apple for 5 years for this exact purpose (adding attributes to existing libraries without changing headers), and only needs to be upstreamed to Clang.

Supporting the forward declarations way is only a few lines of code now, supporting API Notes is a much larger effort. I would also prefer the latter but I think having this work not blocked on upstreaming API notes is essential to get this upstreamed. Or would you prefer not supporting the 3rd party library use-case until API Notes are upstreamed?

I would prefer an API Notes-based approach. And I would suggest that you, or someone else working on these attributes, help with upstreaming.

The reason why I prefer API notes is because users forward declaring third party APIs will put those libraries in a difficult situation, where those libraries can't use normal evolution processes to change their APIs -- the forward declarations that users have will stop compiling, or will start introducing additional overloads which don't have definitions. Of course severity of the problem depends on the nature of such forward declarations -- would it only be for owner and pointer types, or for some other APIs? Just the types -- and rare types like pointers -- is not that bad, however forward declaring functions is bad.

With API notes users have all the extra stuff out of line, and it is easy to disable when things go sideways.

When forward declarations are sprinkled in users' code, they are impossible to change.

[mgehre]

I understand that putting types on forward declared header (standard libraries / third party libraries) is not a approach we should favor.
Thank you for your good arguments.
API notes seems like a really good approach for this (I would like to help with the upstreaming), and in the meanwhile
I will work on hard-coding attributes for standard library types in clang for a follow-up PR. I think we could have them always attached (I will measure performance),
so that -Wlifetime will not change the AST.

[mgehre]

• Add newline at end of file

[mgehre]

• Make the list of attribute properties more consistent.

[xazax.hun]

clang/include/clang/Basic/AttrDocs.td

4177 ↗

(On Diff #207235)

I think it is referenced implicitly after the comma by it. Would you prefer to get rid of the name P and only say a class?

[gribozavr]

clang/include/clang/Basic/AttrDocs.td

4164 ↗

(On Diff #207613)

Slightly better: "In a class annotated with ..., each function that returns ... is assumed to ..."

However, the "is assumed" part throws me off. There's no assumption here -- this is what this attribute means, intentionally, by design, no guessing involved.

So how about this?

The attribute [[gsl::Owner(T)]] applies to structs and classes that own an object of type T:

[[gsl::Owner(int)]] class IntOwner {
private:
  int value;
public:
  int *getInt() { return &value; }
  long *getLong();
};

In a class annotated like this each member function that returns T& or T* returns a pointer/reference to the data owned by the class instance:

IntOwner int_owner;
int *x = int_owner.getInt();
// `x` points to memory owned by `int_owner`.

Therefore, that the lifetime of that data ends when ... For example:

int *x = IntOwner().getInt();
// `x` points to memory owned by the temporary `IntOwner()`, which has been destroyed now.

long *y = IntOwner().getLong();
// Can't make any conclusion about validity of `y`.

4177 ↗

(On Diff #207613)

Ditto, "A class annotated with ... is assumed to be ..."

However, again, I feel like "assume" is the wrong word to use.

"The attribute [[gsl::Pointer(T)]] applies to structs and classes that behave like pointers to an object of type T:

class [[gsl::Pointer(int)]] IntPointer {
private:
  int *valuePointer;
public:
  // fill it in with appropriate APIs
};

Classes annotated like this behave like regular pointers: they can either point to objects of type T, or dangle (<<<explain what it means>>>).

For example: ... sample code and a static analysis message...

clang/lib/Sema/SemaDeclAttr.cpp

4546–4547 ↗

(On Diff #207235)

What about array types? References?

gsl::Owner(int[])
gsl::Owner(int&)

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

I take this to mean that we're not implementing the same attribute as MSVC is, and that worries me.

Always providing the type argument should be compatible with future versions of the MSVC implementation.

The problem is that code written for MSVC (where the type argument is not required) will not compile with Clang (where the type argument is required), if I understand properly.

Generally speaking, when we implement an attribute from another vendor namespace, we expect the attribute to have the same semantics as whoever "owns" that vendor namespace. It's a bit trickier here because gsl isn't really a vendor so much as a collective, so I don't know who is the authority to turn to.

On a completely different note: would this attribute also make sense within C with a C2x spelling?

clang/lib/Parse/ParseDecl.cpp

335–341 ↗

(On Diff #207613)

I don't like how this short-circuits the remainder of the logic in this function. I'd rather see this one-off logic handled in ParseClangAttributeArgs() if we have to treat it as a one-off. A better approach would be to implement type arguments within ParseAttributeArgsCommon().

clang/lib/Sema/SemaDecl.cpp

2618–2619 ↗

(On Diff #207613)

I'd like to see some tests for this.

I'm a bit worried about whether this will do good things in practice or not. I'm concerned about situations like:

struct S;

void func(S *s) {
  // Interesting things happen here.
}

struct [[gsl::Pointer]] S {};

and whether the "interesting things" will be properly [un]diagnosed.

[xazax.hun]

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

I see, that is a valid concern. A followup patch makes the type argument optional so it will be fully compatible with MSVC. I don't think it makes sense with C or at least I am not sure what would I annotate using these attributes in C code as the subjects are classes. I know that it is possible to simulate classes in C but I am not sure how well the analysis would work in such cases.

Are you OK with making the argument optional in a followup patch or do you want us to cherry-pick that modification into this one?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

I don't think it makes sense with C or at least I am not sure what would I annotate using these attributes in C code as the subjects are classes. I know that it is possible to simulate classes in C but I am not sure how well the analysis would work in such cases.

I couldn't think of a reason to expose this in C either, so I think the current spelling is good as-is.

Are you OK with making the argument optional in a followup patch or do you want us to cherry-pick that modification into this one?

I'd prefer the argument be optional in this patch.

[mgehre]

• Don't crash when pretty-printing an Attribute with optional type argument
• Don't crash AST dump when Owner/Pointer has no Type argument
• gsl::Owner/gsl::Pointer: Make DerefType parameter optional
• Unify type and non-type attribute parsing; don't add OwnerAttr/PointerAttr twice
• Improve documentation

clang/include/clang/Basic/Attr.td

2770 ↗

(On Diff #207235)

The argument is now optional.

clang/include/clang/Basic/AttrDocs.td

4164 ↗

(On Diff #207613)

What I meant to express by "is assumed to" is that this attribute does not change what member functions do. E.g. if I had int* f() { static int i; return &i; }, then f would not return data owned by the class instance, even when annotating the class with [[gsl::Owner(T)]]. The point is that when member functions with return type T*/T& don't return data owned by the class instance, one should not add the [[gsl::Owner(T)]] attribute - or the analysis will produce wrong results. Here, we would warn if code uses the returned value of f after the class instances has been destroyed even though it would be safe.

There is a chicken and egg problem here. With this PR, we don't get any analysis yet, so providing examples of analysis results like "x points to memory owned by the temporary IntOwner(), which has been destroyed now." can be misleading. On the other hand, if we would just document the current meaning of the attribute, it would be nothing.
The implication "member function that returns T& or T* returns a pointer/reference to the data owned by the class instance" is just one part of being an Owner. Herb's paper
uses this in more ways (e.g. output arguments, handling of move semantics, etc.), which we would implement incrementally together with the respective documentation update.

So I propose to keep the documentation basic for this PR and update the documentation with the upcoming PRs that add to the analysis. Would that be acceptable?
I added a note that the attribute is currently experimental and subject to change.

clang/lib/Parse/ParseDecl.cpp

335–341 ↗

(On Diff #207613)

I integrated parsing of type attributes now into the main logic of ParseAttributeArgsCommon()

clang/lib/Sema/SemaDecl.cpp

2618–2619 ↗

(On Diff #207613)

I have removed this part of the PR. Implementing implicit attributes for std types went well without requiring to allow "late" additions of attributes.

[gribozavr]

clang/include/clang/Basic/AttrDocs.td

4164 ↗

(On Diff #207613)

What I meant to express by "is assumed to" is that this attribute does not change what member functions do.

I don't think we should be making allowances for code that uses an attribute to promise something and then does some other thing. In that model, there's alignment between what code does and what attribute says.

And I mean of course the attribute does not change what the code does... that must be obvious but you can mention it explicitly if you want.

With this PR, we don't get any analysis yet

I don't think the analysis must be automatic in order to show what analysis can be done. Attribute documentation needs enough examples to give people a flavor of what the attribute does, and what possible benefits it can bring.

Documentation for the specific analysis that is actually done should go elsewhere into Clang's documentation (if necessary).

[mgehre]

• Disallow reference and array types as DerefType
• Add example to documentation.

[mgehre]

I think that I have addressed all open comments.

clang/include/clang/Basic/AttrDocs.td

4164 ↗

(On Diff #207613)

I added an example to the documentation to show how the initial analysis will look like to give a feeling for what the attribute does.

clang/lib/Sema/SemaDeclAttr.cpp

4546–4547 ↗

(On Diff #207235)

Good catch! I disallow them now and added appropriate tests.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4167 ↗

(On Diff #211008)

Do either of these attributes make sense on a union type? If so, might be worth mentioning unions here and below. If not, would it be worth diagnosing on a union?

clang/include/clang/Basic/DiagnosticSemaKinds.td

2516 ↗

(On Diff #211008)

Can you combine this one with err_attribute_argument_vec_type_hint?

clang/lib/Parse/ParseDecl.cpp

385 ↗

(On Diff #211008)

Can you add a FIXME prefix to the comment?

clang/lib/Sema/SemaDeclAttr.cpp

4550 ↗

(On Diff #211008)

This should say a reference type, but I prefer seeing this done in a %select{} within the diagnostic rather than manually adding string literals. This will also simplify the logic down to:

unsigned SelectIdx = ~0U;
if (ParamType->isVoidType())
  SelectIdx = 0;
else if (ParamType->isReferenceType())
  SelectIdx = 1;
else if (ParamType->isArrayType())
  SelectIdx = 2;

if (SelectIdx != ~0U) {
  S.Diag(...) << SelectIdx << AL;
  return;
}

[mgehre]

• Diagnose unions; improve other diagnostics; fix all comments

[mgehre]

clang/include/clang/Basic/AttrDocs.td

4167 ↗

(On Diff #211008)

Good catch! I added diagnostics and a test.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2516 ↗

(On Diff #211008)

I'm not sure: vec_type_hint reads "invalid attribute argument %0 - expecting a vector or vectorizable scalar type" and here ""%0 is an invalid argument to attribute %1", i.e. one is positive ("expecting ...") and the other is negative ("%0 is an invalid argument").

I don't know how to describe "not void, not reference, not array type" in terms of "expecting ...", and I think that we should keep "expecting a vector or vectorizable scalar type" on the VecTypeHint attribute diagnostic.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2771 ↗

(On Diff #211370)

This subject should be Struct and same below.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2516 ↗

(On Diff #211008)

I had figured it would be something like %select{'void'|a reference type|an array type|a non-vector or non-vectorizable scalar type}0 is an invalid argument to attribute %1 but I am not strongly opposed if you want to keep the diagnostics separate.

2933 ↗

(On Diff #211370)

You can drop this change when the Subjects field is fixed above.

clang/include/clang/Sema/ParsedAttr.h

1037 ↗

(On Diff #211370)

Same

clang/lib/Sema/SemaDeclAttr.cpp

4538–4542 ↗

(On Diff #211370)

You can drop this bit when the Subjects field is fixed above.

clang/test/Misc/pragma-attribute-supported-attributes-list.test

119 ↗

(On Diff #211370)

These will also wind up needing to be updated when switching the Subjects field.

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

2 ↗

(On Diff #211370)

This file tests part of parsing and part of Sema somewhat interchangeably. I'm not strictly opposed to it being that way, but it would be a bit cleaner to have the parsing tests in the Parser directory and the rest in SemaCXX (that also reduces the confusion from the file name).

[mgehre]

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

2 ↗

(On Diff #211370)

I had split the tests into two files due to a misconception that the ast-dump would not work when errors are emitted. It actually does work, and so I recombined the test cases.

[mgehre]

• Fix all comments

[aaron.ballman]

LGTM aside from a minor formatting nit. Thank you for working on these attributes!

clang/include/clang/Basic/DiagnosticSemaKinds.td

2517 ↗

(On Diff #211600)

80-col limit

[mgehre]

Thank you very much for dedicating your time for the review. It improved the code a lot!

[gribozavr]

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

4252

"and is currently ignored"

even better:

"and is ignored"

It does not matter for the user that we might change it in future. We might change *anything* in future, and yet we don't hedge everywhere.

4278

ditto

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

34

Technically correct, but not very helpful for the user.

It would be better if the message was tailored for the attribute, for example,

owner types can't own an object of type 'void'

[mgehre]

I will include your latest comments into D64448

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

let Args = [IdentifierArgument<"ArgumentKind">,
               TypeArgument<"MatchingCType">,
               BoolArgument<"LayoutCompatible">,
               BoolArgument<"MustBeNull">];
 //  let Subjects = SubjectList<[Var], ErrorDiag>;
   let HasCustomParsing = 1;
   let Documentation = [TypeTagForDatatypeDocs];
 }
 
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimeOwnerDocs];
+}
+
+def Pointer : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Pointer">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimePointerDocs];
+}
+
 // Microsoft-related attributes
 
 def MSNoVTable : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
   let Spellings = [Declspec<"novtable">];
   let Subjects = SubjectList<[CXXRecord]>;
   let Documentation = [MSNoVTableDocs];
 }
 

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

 The GNU style attribute __attribute__((hip_pinned_shadow)) or MSVC style attribute
 __declspec(hip_pinned_shadow) can be added to the definition of a global variable
 to indicate it is a HIP pinned shadow variable. A HIP pinned shadow variable can
 be accessed on both device side and host side. It has external linkage and is
 not initialized on device side. It has internal linkage and is initialized by
 the initializer on host side.
   }];
 }
+
+def LifetimeOwnerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Owner(T)]]`` applies to structs and classes that own an
+object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Owner(int)]] IntOwner {
+  private:
+    int value;
+  public:
+    int *getInt() { return &value; }
+  };
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code
+generation.
+
+See Pointer_ for an example.
+}];
+}
+
+def LifetimePointerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Pointer(T)]]`` applies to structs and classes that behave
+like pointers to an object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Pointer(int)]] IntPointer {
+  private:
+    int *valuePointer;
+  public:
+    int *getInt() { return &valuePointer; }
+  };
+
+The argument ``T`` is optional and currently 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.
+
+.. code-block:: c++
+
+  int f() {
+    IntPointer P;
+    if (true) {
+      IntOwner O(7);
+      P = IntPointer(O); // P "points into" O
+    } // P is dangling
+    return P.get(); // error: Using a dangling Pointer.
+  }
+
+}];
+}

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

def warn_auto_var_is_id : Warning<
   "'auto' deduced as 'id' in declaration of %0">,
   InGroup<DiagGroup<"auto-var-id">>;
 
 // Attributes
 def err_nsobject_attribute : Error<
   "'NSObject' attribute is for pointer types only">;
 def err_attributes_are_not_compatible : Error<
   "%0 and %1 attributes are not compatible">;
+def err_attribute_invalid_argument : Error<
+  "%select{'void'|a reference type|an array type|a non-vector or "
+  "non-vectorizable scalar type}0 is an invalid argument to attribute %1">;
 def err_attribute_wrong_number_arguments : Error<
   "%0 attribute %plural{0:takes no arguments|1:takes one argument|"
   ":requires exactly %1 arguments}1">;
 def err_attribute_too_many_arguments : Error<
   "%0 attribute takes no more than %1 argument%s1">;
 def err_attribute_too_few_arguments : Error<
   "%0 attribute takes at least %1 argument%s1">;
 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;

 def err_attribute_argument_type : Error<
   "%0 attribute requires %select{int or bool|an integer "
   "constant|a string|an identifier}1">;
 def err_attribute_argument_out_of_range : Error<
   "%0 attribute requires integer constant between %1 and %2 inclusive">;
 def err_init_priority_object_attr : Error<
   "can only use 'init_priority' attribute on file-scope definitions "
   "of objects of class type">;
-def err_attribute_argument_vec_type_hint : Error<
-  "invalid attribute argument %0 - expecting a vector or vectorizable scalar type">;
 def err_attribute_argument_out_of_bounds : Error<
   "%0 attribute parameter %1 is out of bounds">;
 def err_attribute_only_once_per_parameter : Error<
   "%0 attribute can only be applied once per parameter">;
 def err_mismatched_uuid : Error<"uuid does not match previous declaration">;
 def note_previous_uuid : Note<"previous uuid specified here">;
 def warn_attribute_pointers_only : Warning<
   "%0 attribute only applies to%select{| constant}1 pointer arguments">,

cfe/trunk/lib/Parse/ParseDecl.cpp

 unsigned Parser::ParseAttributeArgsCommon(
     IdentifierInfo *AttrName, SourceLocation AttrNameLoc,
     ParsedAttributes &Attrs, SourceLocation *EndLoc, IdentifierInfo *ScopeName,
     SourceLocation ScopeLoc, ParsedAttr::Syntax Syntax) {
   // Ignore the left paren location for now.
   ConsumeParen();
 
   bool ChangeKWThisToIdent = attributeTreatsKeywordThisAsIdentifier(*AttrName);
+  bool AttributeIsTypeArgAttr = attributeIsTypeArgAttr(*AttrName);
 
   // Interpret "kw_this" as an identifier if the attributed requests it.
   if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
     Tok.setKind(tok::identifier);
 
   ArgsVector ArgExprs;
   if (Tok.is(tok::identifier)) {
     // If this attribute wants an 'identifier' argument, make it so.
if (AttrKind == ParsedAttr::UnknownAttribute ||
       const Token &Next = NextToken();
       IsIdentifierArg = Next.isOneOf(tok::r_paren, tok::comma);
     }
 
     if (IsIdentifierArg)
       ArgExprs.push_back(ParseIdentifierLoc());
   }
 
+  ParsedType TheParsedType;
   if (!ArgExprs.empty() ? Tok.is(tok::comma) : Tok.isNot(tok::r_paren)) {
     // Eat the comma.
     if (!ArgExprs.empty())
       ConsumeToken();
 
     // Parse the non-empty comma-separated list of expressions.
     do {
       // Interpret "kw_this" as an identifier if the attributed requests it.
       if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
         Tok.setKind(tok::identifier);
 
       ExprResult ArgExpr;
-      if (Tok.is(tok::identifier) &&
+      if (AttributeIsTypeArgAttr) {
+        TypeResult T = ParseTypeName();
+        if (T.isInvalid()) {
+          SkipUntil(tok::r_paren, StopAtSemi);
+          return 0;
+        }
+        if (T.isUsable())
+          TheParsedType = T.get();
+        break; // FIXME: Multiple type arguments are not implemented.
+      } else if (Tok.is(tok::identifier) &&
                  attributeHasVariadicIdentifierArg(*AttrName)) {
         ArgExprs.push_back(ParseIdentifierLoc());
       } else {
         bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
         EnterExpressionEvaluationContext Unevaluated(
             Actions,
             Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
                    : Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
         ExprResult ArgExpr(
             Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
         if (ArgExpr.isInvalid()) {
           SkipUntil(tok::r_paren, StopAtSemi);
           return 0;
         }
         ArgExprs.push_back(ArgExpr.get());
       }
       // Eat the comma, move to the next argument
     } while (TryConsumeToken(tok::comma));
   }
 
   SourceLocation RParen = Tok.getLocation();
   if (!ExpectAndConsume(tok::r_paren)) {
     SourceLocation AttrLoc = ScopeLoc.isValid() ? ScopeLoc : AttrNameLoc;
+
+    if (AttributeIsTypeArgAttr && !TheParsedType.get().isNull()) {
+      Attrs.addNewTypeAttr(AttrName, SourceRange(AttrNameLoc, RParen),
+                           ScopeName, ScopeLoc, TheParsedType, Syntax);
+    } else {
       Attrs.addNew(AttrName, SourceRange(AttrLoc, RParen), ScopeName, ScopeLoc,
                    ArgExprs.data(), ArgExprs.size(), Syntax);
     }
+  }
 
   if (EndLoc)
     *EndLoc = RParen;
 
-  return static_cast<unsigned>(ArgExprs.size());
+  return static_cast<unsigned>(ArgExprs.size() + !TheParsedType.get().isNull());
 }
 
 /// Parse the arguments to a parameterized GNU attribute or
 /// a C++11 attribute in "gnu" namespace.
 void Parser::ParseGNUAttributeArgs(IdentifierInfo *AttrName,
                                    SourceLocation AttrNameLoc,
                                    ParsedAttributes &Attrs,
                                    SourceLocation *EndLoc,

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

static void handleVecTypeHint(Sema &S, Decl *D, const ParsedAttr &AL) {
 
   TypeSourceInfo *ParmTSI = nullptr;
   QualType ParmType = S.GetTypeFromParser(AL.getTypeArg(), &ParmTSI);
   assert(ParmTSI && "no type source info for attribute argument");
 
   if (!ParmType->isExtVectorType() && !ParmType->isFloatingType() &&
       (ParmType->isBooleanType() ||
        !ParmType->isIntegralType(S.getASTContext()))) {
-    S.Diag(AL.getLoc(), diag::err_attribute_argument_vec_type_hint)
-        << ParmType;
+    S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument) << 3 << AL;
     return;
   }
 
   if (VecTypeHintAttr *A = D->getAttr<VecTypeHintAttr>()) {
     if (!S.Context.hasSameType(A->getTypeHint(), ParmType)) {
       S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;
       return;
     }
for (unsigned I = 0, E = AL.getNumArgs(); I != E; ++I) {
     // clang-tidy knows about available rules.
     DiagnosticIdentifiers.push_back(RuleName);
   }
   D->addAttr(::new (S.Context) SuppressAttr(
       AL.getRange(), S.Context, DiagnosticIdentifiers.data(),
       DiagnosticIdentifiers.size(), AL.getAttributeSpellingListIndex()));
 }
 
+static void handleLifetimeCategoryAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  TypeSourceInfo *DerefTypeLoc = nullptr;
+  QualType ParmType;
+  if (AL.hasParsedType()) {
+    ParmType = S.GetTypeFromParser(AL.getTypeArg(), &DerefTypeLoc);
+
+    unsigned SelectIdx = ~0U;
+    if (ParmType->isVoidType())
+      SelectIdx = 0;
+    else if (ParmType->isReferenceType())
+      SelectIdx = 1;
+    else if (ParmType->isArrayType())
+      SelectIdx = 2;
+
+    if (SelectIdx != ~0U) {
+      S.Diag(AL.getLoc(), diag::err_attribute_invalid_argument)
+          << SelectIdx << AL;
+      return;
+    }
+  }
+
+  // To check if earlier decl attributes do not conflict the newly parsed ones
+  // we always add (and check) the attribute to the cannonical decl.
+  D = D->getCanonicalDecl();
+  if (AL.getKind() == ParsedAttr::AT_Owner) {
+    if (checkAttrMutualExclusion<PointerAttr>(S, D, AL))
+      return;
+    if (const auto *OAttr = D->getAttr<OwnerAttr>()) {
+      const Type *ExistingDerefType = OAttr->getDerefTypeLoc()
+                                          ? OAttr->getDerefType().getTypePtr()
+                                          : nullptr;
+      if (ExistingDerefType != ParmType.getTypePtrOrNull()) {
+        S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible)
+            << AL << OAttr;
+        S.Diag(OAttr->getLocation(), diag::note_conflicting_attribute);
+      }
+      return;
+    }
+    D->addAttr(::new (S.Context)
+                   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+                             AL.getAttributeSpellingListIndex()));
+  } else {
+    if (checkAttrMutualExclusion<OwnerAttr>(S, D, AL))
+      return;
+    if (const auto *PAttr = D->getAttr<PointerAttr>()) {
+      const Type *ExistingDerefType = PAttr->getDerefTypeLoc()
+                                          ? PAttr->getDerefType().getTypePtr()
+                                          : nullptr;
+      if (ExistingDerefType != ParmType.getTypePtrOrNull()) {
+        S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible)
+            << AL << PAttr;
+        S.Diag(PAttr->getLocation(), diag::note_conflicting_attribute);
+      }
+      return;
+    }
+    D->addAttr(::new (S.Context)
+                   PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+                               AL.getAttributeSpellingListIndex()));
+  }
+}
+
 bool Sema::CheckCallingConvAttr(const ParsedAttr &Attrs, CallingConv &CC,
                                 const FunctionDecl *FD) {
   if (Attrs.isInvalid())
     return true;
 
   if (Attrs.hasProcessingCache()) {
     CC = (CallingConv) Attrs.getProcessingCache();
     return false;
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   case ParsedAttr::AT_PreserveMost:
   case ParsedAttr::AT_PreserveAll:
   case ParsedAttr::AT_AArch64VectorPcs:
     handleCallConvAttr(S, D, AL);
     break;
   case ParsedAttr::AT_Suppress:
     handleSuppressAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_Owner:
+  case ParsedAttr::AT_Pointer:
+    handleLifetimeCategoryAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_OpenCLKernel:
     handleSimpleAttribute<OpenCLKernelAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_OpenCLAccess:
     handleOpenCLAccessAttr(S, D, AL);
     break;
   case ParsedAttr::AT_OpenCLNoSVM:
     handleOpenCLNoSVMAttr(S, D, AL);

cfe/trunk/test/AST/ast-dump-attr.cpp

void f() {
       // CHECK: AttributedStmt
       // CHECK-NEXT: SuppressAttr{{.*}} on-stmt
       // CHECK-NEXT: CompoundStmt
         i = reinterpret_cast<int*>(7);
       }
     }
 }
 
+namespace TestLifetimeCategories {
+class [[gsl::Owner(int)]] AOwner{};
+// CHECK: CXXRecordDecl{{.*}} class AOwner
+// CHECK: OwnerAttr {{.*}} int
+class [[gsl::Pointer(int)]] APointer{};
+// CHECK: CXXRecordDecl{{.*}} class APointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument
+// CHECK: PointerAttr
+
+class [[gsl::Owner]] OwnerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class OwnerWithoutArgument
+// CHECK: OwnerAttr
+} // namespace TestLifetimeCategories
+
 // Verify the order of attributes in the Ast. It must reflect the order
 // in the parsed source.
 int mergeAttrTest() __attribute__((deprecated)) __attribute__((warn_unused_result));
 int mergeAttrTest() __attribute__((annotate("test")));
 int mergeAttrTest() __attribute__((unused,no_thread_safety_analysis));
 // CHECK: FunctionDecl{{.*}} mergeAttrTest
 // CHECK-NEXT: DeprecatedAttr
 // CHECK-NEXT: WarnUnusedResultAttr

cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: ObjCRootClass (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCRuntimeName (SubjectMatchRule_objc_interface, SubjectMatchRule_objc_protocol)
 // CHECK-NEXT: ObjCRuntimeVisible (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
+// CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)

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

+// RUN: %clang_cc1 -verify -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+union [[gsl::Owner(int)]] Union{};
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} BothOwnerPointer
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater
+// CHECK: PointerAttr {{.*}} int
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-5 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater
+// CHECK: PointerAttr {{.*}} Inherited int
+
+class [[gsl::Owner(int)]] AddConflictLater2{};
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater2
+// CHECK: OwnerAttr {{.*}} int
+class [[gsl::Owner(float)]] AddConflictLater2;
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-5 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater
+// CHECK: OwnerAttr {{.*}} Inherited int
+
+class [[gsl::Owner()]] [[gsl::Owner(int)]] WithAndWithoutParameter{};
+// expected-error@-1 {{'Owner' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} WithAndWithoutParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Owner(int &)]] ReferenceType{};
+// expected-error@-1 {{a reference type is an invalid argument to attribute 'Owner'}}
+
+class [[gsl::Pointer(int[])]] ArrayType{};
+// expected-error@-1 {{an array type is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} OwnerMissingParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Pointer]] PointerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} PointerMissingParameter
+// CHECK: PointerAttr
+
+class [[gsl::Owner()]] OwnerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} OwnerWithEmptyParameterList
+// CHECK: OwnerAttr {{.*}}
+
+class [[gsl::Pointer()]] PointerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} PointerWithEmptyParameterList
+// CHECK: PointerAttr {{.*}}
+
+struct [[gsl::Owner(int)]] AnOwner{};
+// CHECK: CXXRecordDecl {{.*}} AnOwner
+// CHECK: OwnerAttr {{.*}} int
+
+struct S;
+class [[gsl::Pointer(S)]] APointer{};
+// CHECK: CXXRecordDecl {{.*}} APointer
+// CHECK: PointerAttr {{.*}} S
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+// CHECK: CXXRecordDecl {{.*}} DuplicateOwner
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+// CHECK: CXXRecordDecl {{.*}} DuplicatePointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+// CHECK: CXXRecordDecl {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater;
+// CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int

cfe/trunk/test/SemaOpenCL/invalid-kernel-attrs.cl

-// RUN: %clang_cc1 -verify %s 
+// RUN: %clang_cc1 -verify %s
 
 kernel __attribute__((vec_type_hint)) void kernel1() {} //expected-error{{'vec_type_hint' attribute takes one argument}}
 
 kernel __attribute__((vec_type_hint(not_type))) void kernel2() {} //expected-error{{unknown type name 'not_type'}}
 
-kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{invalid attribute argument 'void' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
-kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{invalid attribute argument 'bool' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
 kernel __attribute__((vec_type_hint(int))) __attribute__((vec_type_hint(float))) void kernel5() {} //expected-warning{{attribute 'vec_type_hint' is already applied with different parameters}}
 
 kernel __attribute__((work_group_size_hint(8,16,32,4))) void kernel6() {} //expected-error{{'work_group_size_hint' attribute requires exactly 3 arguments}}
 
 kernel __attribute__((work_group_size_hint(1,2,3))) __attribute__((work_group_size_hint(3,2,1))) void kernel7() {}  //expected-warning{{attribute 'work_group_size_hint' is already applied with different parameters}}
 
 __attribute__((reqd_work_group_size(8,16,32))) void kernel8(){} // expected-error {{attribute 'reqd_work_group_size' can only be applied to an OpenCL kernel}}

cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

public:
     void writePCHWrite(raw_ostream &OS) const override {
       OS << "    " << WritePCHRecord(type, "SA->get" +
                                            std::string(getUpperName()) + "()");
     }
 
     std::string getIsOmitted() const override {
       if (type == "IdentifierInfo *")
         return "!get" + getUpperName().str() + "()";
+      if (type == "TypeSourceInfo *")
+        return "!get" + getUpperName().str() + "Loc()";
       if (type == "ParamIdx")
         return "!get" + getUpperName().str() + "().isValid()";
       return "false";
     }
 
     void writeValue(raw_ostream &OS) const override {
       if (type == "FunctionDecl *")
         OS << "\" << get" << getUpperName()
void writeDump(raw_ostream &OS) const override {
         OS << "    dumpBareDeclRef(SA->get" << getUpperName() << "());\n"; 
       } else if (type == "IdentifierInfo *") {
         // Some non-optional (comma required) identifier arguments can be the
         // empty string but are then recorded as a nullptr.
         OS << "    if (SA->get" << getUpperName() << "())\n"
            << "      OS << \" \" << SA->get" << getUpperName()
            << "()->getName();\n";
       } else if (type == "TypeSourceInfo *") {
+        if (isOptional())
+          OS << "    if (SA->get" << getUpperName() << "Loc())";
         OS << "    OS << \" \" << SA->get" << getUpperName()
            << "().getAsString();\n";
       } else if (type == "bool") {
         OS << "    if (SA->get" << getUpperName() << "()) OS << \" "
            << getUpperName() << "\";\n";
       } else if (type == "int" || type == "unsigned") {
         OS << "    OS << \" \" << SA->get" << getUpperName() << "();\n";
       } else if (type == "ParamIdx") {