[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
   /// Attrs - Attributes.
   ParsedAttributes Attrs;
 
+  ParsedAttributes DeclarationAttrs;

mboehme wrote:
> aaron.ballman wrote:
> > We should probably rename `Attrs` to be less generic and add some comments 
> > to explain why there are two lists of parsed attributes.
> I've added some comments to clarify the difference. I'm not sure what a good 
> alternative name for `Attrs` would be though. The only obvious candidate that 
> comes to mind is `DeclaratorAttrs`, but that seems pretty redundant given 
> that this is a field of `Declarator`.
> 
> I think there is actually a good case for calling the first list simply 
> `Attrs` because, unlike `DeclarationAttrs`, it applies directly to the 
> `Declarator`. This distinction is amplified by the fact that, since your 
> review, I've changed `DeclarationAttrs` to be a reference, not a by-value 
> field.
The new comment actually makes it less clear for me -- that says the attributes 
are for the `DeclSpec` but they're actually for the `Declatator` (there's no 
relationship between `Attrs` and `DS`).

Given that `DeclSpec` uses `Attrs` for its associated attributes, I think it's 
okay to use `Attrs` here for the ones associated with the `Declarator`, but the 
comment should be updated in that case.



Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+  /// This may only be called if isStandardAttributeSyntax() returns true.
+  bool slidesFromDeclToDeclSpec() const;
+

Because this is now specific to standard attributes, and those don't "slide" 
under normal circumstances, it might be nice to rename this method to make it 
more clear that this should not be called normally. I don't have a strong 
opinion on what a *good* name is, but even something like 
`improperlySlidesFromDeclToDeclSpec()` would make me happy.



Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ProhibitAttributes(DeclAttrs);
+ProhibitAttributes(OriginalDeclSpecAttrs);
 DeclEnd = Tok.getLocation();

rsmith wrote:
> This looks wrong to me (both before and after this patch; you've faithfully 
> retained an incorrect behavior). If `DeclSpec` attributes aren't allowed, 
> they should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after 
> this patch, we'll diagnose attributes in a free-standing decl-specifier-seq 
> if we happened to tentatively parse them, not if we happened to parse them in 
> `ParseDeclarationSpecifiers`. For example:
> 
> ```
> __attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
> void f() {
>   __attribute(()) struct X; // DeclSpecAttrs will contain the attribute 
> specifier, error
> }
> ```
> 
> Comparing to GCC's behavior for that example (which GCC silently accepts) and 
> for this one:
> 
> ```
> __attribute__((packed)) struct X; // GCC warns that packed has no meaning 
> here, clang produces a high-quality warning.
> void f() {
>   __attribute((packed)) struct X; // GCC warns that packed has no meaning 
> here, clang produces a bogus error.
> }
> ```
> 
> ... I think the right thing to do is to delete this call (along with 
> `OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently 
> syntactically permitted here, but most (maybe even all?) GNU attributes don't 
> have any meaning in this position.
Good catch!



Comment at: clang/lib/Parse/ParseDecl.cpp:2188
 // Parse the next declarator.
-D.clear();
+D.clearExceptDeclarationAttrs();
 D.setCommaLoc(CommaLoc);

mboehme wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I wonder if a name like `prepareForNextDeclarator` or `clearForComma` 
> > > would be better here -- something that indicates why we're clearing 
> > > rather than describing how.
> > Personally, I like `prepareForNextDeclarator()` -- that's nice and clear 
> > (to me).
> Now that `Declarator` only holds a reference to the declaration attributes, 
> the additional `clearExceptDeclarationAttrs()` has become unnecessary. With 
> that, I think the original `clear()` name is acceptable?
It's fine by me.



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2716
 // Otherwise, it must be a using-declaration or an alias-declaration.
 return ParseUsingDeclaration(DeclaratorContext::Member, TemplateInfo,
+ UsingLoc, DeclEnd, DeclAttrs, AS);

mboehme wrote:
> rsmith wrote:
> > Do we need to `ProhibitAttrs(DeclSpecAttrs)` along this code path? For 
> > example:
> > ```
> > struct A { int a; };
> > struct B : A { [uuid("1234")] using A::a; };
> > ```
> > ... currently warns (for `-target x86_64-windows`) but I think with this 
> > patch we'll silently drop the `uuid` attribute on the floor.
> Good point.
> 
> Instead of doing a `ProhibitAttributes()`, I've decided to simply move the 
> 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'm very happy with how this patch is looking.




Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr ) {
+  return AL.isStandardAttributeSyntax();
+}));

rsmith wrote:
> I think I recall seeing OpenMP pragma attributes being parsed as declaration 
> attributes too. FIXME update this comment after review is complete
Oops, forgot to circle back to this. I think I was wrong; please disregard :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr ) {
+  return AL.isStandardAttributeSyntax();
+}));

I think I recall seeing OpenMP pragma attributes being parsed as declaration 
attributes too. FIXME update this comment after review is complete



Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ProhibitAttributes(DeclAttrs);
+ProhibitAttributes(OriginalDeclSpecAttrs);
 DeclEnd = Tok.getLocation();

This looks wrong to me (both before and after this patch; you've faithfully 
retained an incorrect behavior). If `DeclSpec` attributes aren't allowed, they 
should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after this 
patch, we'll diagnose attributes in a free-standing decl-specifier-seq if we 
happened to tentatively parse them, not if we happened to parse them in 
`ParseDeclarationSpecifiers`. For example:

```
__attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
void f() {
  __attribute(()) struct X; // DeclSpecAttrs will contain the attribute 
specifier, error
}
```

Comparing to GCC's behavior for that example (which GCC silently accepts) and 
for this one:

```
__attribute__((packed)) struct X; // GCC warns that packed has no meaning here, 
clang produces a high-quality warning.
void f() {
  __attribute((packed)) struct X; // GCC warns that packed has no meaning here, 
clang produces a bogus error.
}
```

... I think the right thing to do is to delete this call (along with 
`OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently 
syntactically permitted here, but most (maybe even all?) GNU attributes don't 
have any meaning in this position.



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2691-2695
+  // We need to keep these attributes for future diagnostics
+  // before they are taken over by the declaration.
+  ParsedAttributesView FnAttrs;
+  FnAttrs.addAll(DeclAttrs.begin(), DeclAttrs.end());
+  FnAttrs.Range = DeclAttrs.Range;

Oh, neat, I think this copy isn't necessary any more, given that we don't give 
ownership of the `DeclAttrs` to the `ParsingDeclarator` any more, and don't mix 
the declaration attributes and the `DeclSpec` attributes together any more. We 
should be able to just use `DeclAttrs` instead of `FnAttrs` below now, I think?



Comment at: clang/lib/Parse/ParseExprCXX.cpp:2745-2746
   // ptr-operators.
-  Declarator D(DS, DeclaratorContext::ConversionId);
+  ParsedAttributes EmptyDeclAttrs(AttrFactory);
+  Declarator D(DS, EmptyDeclAttrs, DeclaratorContext::ConversionId);
   ParseDeclaratorInternal(D, /*DirectDeclParser=*/nullptr);

Given the number of times this has come up, I wonder if it's worth adding 
something like:

```
class ParsedAttributes {
public:
  // ...
  static const ParsedAttributes () {
static AttributeFactory Factory;
static AttributePool Pool(Factory);
static const ParsedAttributes Attrs(Pool);
return Attrs;
  }
  // ...
};
```
(or, better, changing `Declarator` to hold a `const ParsedAttributesView&` 
rather than a `const ParsedAttributes&`) so that we can write
```
Declarator D(DS, ParsedAttributes::none(), DeclaratorContext::ConversionId);
```

Totally optional, though, and I'm happy for this cleanup to happen at some 
later point in time.



Comment at: clang/lib/Parse/ParseStmt.cpp:16
 #include "clang/Basic/Attributes.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/PrettyStackTrace.h"

This header is intended to be private to Sema. If you want to use a diagnostic 
in both the parser and sema, please move it to `DiagnosticCommonKinds.td`.



Comment at: clang/lib/Parse/ParseStmt.cpp:242-246
+Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+GNUAttrs, );
   } else {
-Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, Attrs);
+Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+GNUAttrs);

mboehme wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I think this is the only place where we're passing decl-specifier-seq 
> > > attributes into `ParseDeclaration`. There are only two possible cases 
> > > here:
> > > 
> > > 1) We have a simple-declaration, and can `ParseSimpleDeclaration` 
> > > directly.
> > > 2) We have a static-assert, using, or namespace alias declaration, which 
> > > don't permit attributes at all.
> > > 
> > > So I think we *could* simplify this so that decl-spec attributes are 
> > > never passed into `ParseDeclaration`:
> > > 
> > > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, 
> > > then prohibit attributes and call `ParseDeclaration`.
> > > * Otherwise, call 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 6 inline comments as done.
mboehme added a comment.

Thank you @rsmith and @aaron.ballman for the detailed review comments!

I hope I've addressed everything -- do let me know if there's anything I've 
missed.

In D126061#3528966 , @rsmith wrote:

> This direction looks good to me. Thanks!
>
> Regarding the possibility of storing the declaration attributes on the 
> `DeclSpec` rather than on the `Declarator`, I wonder if a better choice might 
> be to make the `Declarator` hold a non-owning reference to the attributes, 
> like it does for the `DeclSpec`?

Thanks for the suggestion -- this has turned out to make the code simpler and 
more logical in a number of places.




Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
   /// Attrs - Attributes.
   ParsedAttributes Attrs;
 
+  ParsedAttributes DeclarationAttrs;

aaron.ballman wrote:
> We should probably rename `Attrs` to be less generic and add some comments to 
> explain why there are two lists of parsed attributes.
I've added some comments to clarify the difference. I'm not sure what a good 
alternative name for `Attrs` would be though. The only obvious candidate that 
comes to mind is `DeclaratorAttrs`, but that seems pretty redundant given that 
this is a field of `Declarator`.

I think there is actually a good case for calling the first list simply `Attrs` 
because, unlike `DeclarationAttrs`, it applies directly to the `Declarator`. 
This distinction is amplified by the fact that, since your review, I've changed 
`DeclarationAttrs` to be a reference, not a by-value field.



Comment at: clang/include/clang/Sema/DeclSpec.h:1975-1984
   /// Reset the contents of this Declarator.
   void clear() {
+clearExceptDeclarationAttrs();
+DeclarationAttrs.clear();
+  }
+
+  /// Reset the contents of this Declarator, except for the declaration

rsmith wrote:
> Note that neither of these functions clear the `DeclSpec`. I suppose that 
> makes sense given that the `Declarator` does not own the `DeclSpec` but 
> merely holds a reference to it. Perhaps we could do the same thing with the 
> declaration attributes?
This is a great idea that also helps with other things. It's similar in spirit 
to the idea that I'd floated in our discussion on 
https://reviews.llvm.org/D111548 of introducing a `Declaration` and having 
`Declarator` hold a reference to that, but it seemed a bit excessive to 
introduce a new `Declaration` class solely for the purpose of holding the 
declaration attributes. Holding a reference directly to the declaration 
attributes achieves the same thing, but without having to introduce yet another 
class. I like it.

One implication of this is that we should only have a const version of 
`getDeclarationAttributes()`; a non-const version would be dangerous because 
we're potentially sharing the attribute list with other declarators. (In fact, 
this wasn't really sound before either because we were effectively already 
sharing the attribute list too by moving it around between declarators.) 
Fortunately, it turns out that all of the places that were using the non-const 
`getDeclarationAttributes()` didn't actually need to be manipulating the 
declaration attribute list. I've added source code comments to call this out 
where appropriate.



Comment at: clang/include/clang/Sema/ParsedAttr.h:657-658
+  /// "slides" to the decl-specifier-seq).
+  /// Attributes with GNU, __declspec or keyword syntax generally slide
+  /// to the decl-specifier-seq. C++11 attributes specified ahead of the
+  /// declaration always appertain to the declaration according to the 
standard,

aaron.ballman wrote:
> rsmith wrote:
> > I don't think this sentence is correct: "Attributes with GNU, __declspec or 
> > keyword syntax generally slide to the decl-specifier-seq." Instead, I think 
> > those attribute syntaxes are never parsed as declaration attributes in the 
> > first place, so there is no possibility of "sliding" anywhere -- they 
> > simply always are decl-spec attributes.
> That's fair -- I tend to think "sliding" is also what happens (at least 
> notionally) in a case like `__attribute__((address_space(1))) int *i;` 
> because it's a type attribute rather than a declaration attribute, but that's 
> also a declaration specifier, so it doesn't really slide anywhere.
Yes, this was inteded to refer to the fact that non-[[]] attributes also 
notionally slide to the `DeclSpec`. In the initial version of my patch, it was 
possible for `slidesFromDeclToDeclSpec()` to be called on non-[[]] attributes 
within the default case in `ProcessDeclAttribute()`.

However, I've realized that this is confusing; it's better to restrict this 
function only to [[]] attributes and to do an explicit check in 
`ProcessDeclAttribute()` to see whether we're looking at a [[]] attribute. I've 
added documentation to this function 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 431948.
mboehme marked 30 inline comments as done.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/MicrosoftExtensions.cpp
  clang/test/Parser/asm.c
  clang/test/Parser/asm.cpp
  clang/test/Parser/c2x-attributes.c
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)\n";
   OS << "<< AL << D->getLocation();\n";
   OS << "  return false;\n";
   OS << "}\n\n";
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -266,9 +266,9 @@
   __attribute__((opencl_private)) private_int_t var5;  // expected-warning {{multiple identical address spaces specified for type}}
   __attribute__((opencl_private)) private_int_t *var6; // expected-warning {{multiple identical address spaces specified for type}}
 #if __OPENCL_CPP_VERSION__
-  [[clang::opencl_private]] __global int var7; // expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] __global int *var8;// expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t var9;// expected-warning {{multiple identical address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t *var10;  // expected-warning {{multiple identical address spaces specified for type}}
+  __global int [[clang::opencl_private]] var7; // expected-error {{multiple address spaces specified for type}}
+  __global int [[clang::opencl_private]] *var8;// expected-error {{multiple address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] var9;// expected-warning {{multiple identical address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] *var10;  // expected-warning {{multiple identical address spaces specified for type}}
 #endif // !__OPENCL_CPP_VERSION__
 }
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- clang/test/SemaCXX/annotate-type.cpp
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -2,10 +2,7 @@
 
 struct S1 {
   void f() [[clang::annotate_type("foo")]];
-  // FIXME: We would want to prohibit the attribute in the following location.
-  // However, Clang currently generally doesn't prohibit type-only C++11
-  // attributes on declarations. This should be fixed more generally.
-  [[clang::annotate_type("foo")]] void g();
+  [[clang::annotate_type("foo")]] void g(); // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
 };
 
 template  struct is_same {
@@ -48,23 +45,21 @@
 
 // More error cases: Prohibit adding the attribute to declarations.
 // Different declarations hit different code paths, so they need separate tests.
-// FIXME: Clang currently generally doesn't prohibit type-only C++11
-// attributes on declarations.
-namespace [[clang::annotate_type("foo")]] my_namespace {}
-struct [[clang::annotate_type("foo")]] S3;
-struct [[clang::annotate_type("foo")]] S3{
-  [[clang::annotate_type("foo")]] int member;
+namespace [[clang::annotate_type("foo")]] my_namespace {} // expected-error {{'annotate_type' 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 431929.
mboehme marked 2 inline comments as done.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/MicrosoftExtensions.cpp
  clang/test/Parser/asm.c
  clang/test/Parser/asm.cpp
  clang/test/Parser/c2x-attributes.c
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)\n";
   OS << "<< AL << D->getLocation();\n";
   OS << "  return false;\n";
   OS << "}\n\n";
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -266,9 +266,9 @@
   __attribute__((opencl_private)) private_int_t var5;  // expected-warning {{multiple identical address spaces specified for type}}
   __attribute__((opencl_private)) private_int_t *var6; // expected-warning {{multiple identical address spaces specified for type}}
 #if __OPENCL_CPP_VERSION__
-  [[clang::opencl_private]] __global int var7; // expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] __global int *var8;// expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t var9;// expected-warning {{multiple identical address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t *var10;  // expected-warning {{multiple identical address spaces specified for type}}
+  __global int [[clang::opencl_private]] var7; // expected-error {{multiple address spaces specified for type}}
+  __global int [[clang::opencl_private]] *var8;// expected-error {{multiple address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] var9;// expected-warning {{multiple identical address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] *var10;  // expected-warning {{multiple identical address spaces specified for type}}
 #endif // !__OPENCL_CPP_VERSION__
 }
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- clang/test/SemaCXX/annotate-type.cpp
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -2,10 +2,7 @@
 
 struct S1 {
   void f() [[clang::annotate_type("foo")]];
-  // FIXME: We would want to prohibit the attribute in the following location.
-  // However, Clang currently generally doesn't prohibit type-only C++11
-  // attributes on declarations. This should be fixed more generally.
-  [[clang::annotate_type("foo")]] void g();
+  [[clang::annotate_type("foo")]] void g(); // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
 };
 
 template  struct is_same {
@@ -48,23 +45,21 @@
 
 // More error cases: Prohibit adding the attribute to declarations.
 // Different declarations hit different code paths, so they need separate tests.
-// FIXME: Clang currently generally doesn't prohibit type-only C++11
-// attributes on declarations.
-namespace [[clang::annotate_type("foo")]] my_namespace {}
-struct [[clang::annotate_type("foo")]] S3;
-struct [[clang::annotate_type("foo")]] S3{
-  [[clang::annotate_type("foo")]] int member;
+namespace [[clang::annotate_type("foo")]] my_namespace {} // expected-error {{'annotate_type' 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 431923.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/MicrosoftExtensions.cpp
  clang/test/Parser/asm.c
  clang/test/Parser/asm.cpp
  clang/test/Parser/c2x-attributes.c
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)\n";
   OS << "<< AL << D->getLocation();\n";
   OS << "  return false;\n";
   OS << "}\n\n";
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -266,9 +266,9 @@
   __attribute__((opencl_private)) private_int_t var5;  // expected-warning {{multiple identical address spaces specified for type}}
   __attribute__((opencl_private)) private_int_t *var6; // expected-warning {{multiple identical address spaces specified for type}}
 #if __OPENCL_CPP_VERSION__
-  [[clang::opencl_private]] __global int var7; // expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] __global int *var8;// expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t var9;// expected-warning {{multiple identical address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t *var10;  // expected-warning {{multiple identical address spaces specified for type}}
+  __global int [[clang::opencl_private]] var7; // expected-error {{multiple address spaces specified for type}}
+  __global int [[clang::opencl_private]] *var8;// expected-error {{multiple address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] var9;// expected-warning {{multiple identical address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] *var10;  // expected-warning {{multiple identical address spaces specified for type}}
 #endif // !__OPENCL_CPP_VERSION__
 }
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- clang/test/SemaCXX/annotate-type.cpp
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -2,10 +2,7 @@
 
 struct S1 {
   void f() [[clang::annotate_type("foo")]];
-  // FIXME: We would want to prohibit the attribute in the following location.
-  // However, Clang currently generally doesn't prohibit type-only C++11
-  // attributes on declarations. This should be fixed more generally.
-  [[clang::annotate_type("foo")]] void g();
+  [[clang::annotate_type("foo")]] void g(); // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
 };
 
 template  struct is_same {
@@ -48,23 +45,21 @@
 
 // More error cases: Prohibit adding the attribute to declarations.
 // Different declarations hit different code paths, so they need separate tests.
-// FIXME: Clang currently generally doesn't prohibit type-only C++11
-// attributes on declarations.
-namespace [[clang::annotate_type("foo")]] my_namespace {}
-struct [[clang::annotate_type("foo")]] S3;
-struct [[clang::annotate_type("foo")]] S3{
-  [[clang::annotate_type("foo")]] int member;
+namespace [[clang::annotate_type("foo")]] my_namespace {} // expected-error {{'annotate_type' attribute cannot be applied to a 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 431921.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/MicrosoftExtensions.cpp
  clang/test/Parser/asm.c
  clang/test/Parser/asm.cpp
  clang/test/Parser/c2x-attributes.c
  clang/test/Parser/cxx0x-attributes.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)\n";
   OS << "<< AL << D->getLocation();\n";
   OS << "  return false;\n";
   OS << "}\n\n";
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -266,9 +266,9 @@
   __attribute__((opencl_private)) private_int_t var5;  // expected-warning {{multiple identical address spaces specified for type}}
   __attribute__((opencl_private)) private_int_t *var6; // expected-warning {{multiple identical address spaces specified for type}}
 #if __OPENCL_CPP_VERSION__
-  [[clang::opencl_private]] __global int var7; // expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] __global int *var8;// expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t var9;// expected-warning {{multiple identical address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t *var10;  // expected-warning {{multiple identical address spaces specified for type}}
+  __global int [[clang::opencl_private]] var7; // expected-error {{multiple address spaces specified for type}}
+  __global int [[clang::opencl_private]] *var8;// expected-error {{multiple address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] var9;// expected-warning {{multiple identical address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] *var10;  // expected-warning {{multiple identical address spaces specified for type}}
 #endif // !__OPENCL_CPP_VERSION__
 }
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- clang/test/SemaCXX/annotate-type.cpp
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -2,10 +2,7 @@
 
 struct S1 {
   void f() [[clang::annotate_type("foo")]];
-  // FIXME: We would want to prohibit the attribute in the following location.
-  // However, Clang currently generally doesn't prohibit type-only C++11
-  // attributes on declarations. This should be fixed more generally.
-  [[clang::annotate_type("foo")]] void g();
+  [[clang::annotate_type("foo")]] void g(); // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
 };
 
 template  struct is_same {
@@ -48,23 +45,21 @@
 
 // More error cases: Prohibit adding the attribute to declarations.
 // Different declarations hit different code paths, so they need separate tests.
-// FIXME: Clang currently generally doesn't prohibit type-only C++11
-// attributes on declarations.
-namespace [[clang::annotate_type("foo")]] my_namespace {}
-struct [[clang::annotate_type("foo")]] S3;
-struct [[clang::annotate_type("foo")]] S3{
-  [[clang::annotate_type("foo")]] int member;
+namespace [[clang::annotate_type("foo")]] my_namespace {} // expected-error {{'annotate_type' attribute cannot be applied to a 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 431919.
mboehme added a comment.

Changes in response to review comments.

One major change is that Declaration now has a reference to the declaration
attributes instead of owning them by value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)\n";
   OS << "<< AL << D->getLocation();\n";
   OS << "  return false;\n";
   OS << "}\n\n";
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -266,9 +266,9 @@
   __attribute__((opencl_private)) private_int_t var5;  // expected-warning {{multiple identical address spaces specified for type}}
   __attribute__((opencl_private)) private_int_t *var6; // expected-warning {{multiple identical address spaces specified for type}}
 #if __OPENCL_CPP_VERSION__
-  [[clang::opencl_private]] __global int var7; // expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] __global int *var8;// expected-error {{multiple address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t var9;// expected-warning {{multiple identical address spaces specified for type}}
-  [[clang::opencl_private]] private_int_t *var10;  // expected-warning {{multiple identical address spaces specified for type}}
+  __global int [[clang::opencl_private]] var7; // expected-error {{multiple address spaces specified for type}}
+  __global int [[clang::opencl_private]] *var8;// expected-error {{multiple address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] var9;// expected-warning {{multiple identical address spaces specified for type}}
+  private_int_t [[clang::opencl_private]] *var10;  // expected-warning {{multiple identical address spaces specified for type}}
 #endif // !__OPENCL_CPP_VERSION__
 }
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- clang/test/SemaCXX/annotate-type.cpp
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -2,10 +2,7 @@
 
 struct S1 {
   void f() [[clang::annotate_type("foo")]];
-  // FIXME: We would want to prohibit the attribute in the following location.
-  // However, Clang currently generally doesn't prohibit type-only C++11
-  // attributes on declarations. This should be fixed more generally.
-  [[clang::annotate_type("foo")]] void g();
+  [[clang::annotate_type("foo")]] void g(); // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
 };
 
 template  struct is_same {
@@ -48,23 +45,21 @@
 
 // More error cases: Prohibit adding the attribute to declarations.
 // Different declarations hit different code paths, so they need separate tests.
-// FIXME: Clang currently generally doesn't prohibit type-only C++11
-// attributes on declarations.
-namespace [[clang::annotate_type("foo")]] my_namespace {}
-struct [[clang::annotate_type("foo")]] S3;
-struct [[clang::annotate_type("foo")]] S3{
-  [[clang::annotate_type("foo")]] int member;
+namespace [[clang::annotate_type("foo")]] my_namespace {} // expected-error {{'annotate_type' attribute cannot be applied to a declaration}}
+struct [[clang::annotate_type("foo")]] S3; // 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118
+  ProcessDeclAttributeOptions Options;
+  Options.IncludeCXX11Attributes = AL.isCXX11Attribute();
+  ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options);

rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > This seems to be equivalent to setting `IncludeCXX11Attributes` to 
> > > `true`, which seems to be equivalent to not setting it at all.
> > Hmmm, not quite -- `AL.isCXX11Attribute()` may return `false` (like for the 
> > GNU spelling of this attribute).
> It might, but we only care about the value of `IncludeCXX11Attributes` when 
> processing a C++11 attribute, so it doesn't matter how this flag is set for a 
> non-C++11 attribute.
Oh, good observation, you're right!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118
+  ProcessDeclAttributeOptions Options;
+  Options.IncludeCXX11Attributes = AL.isCXX11Attribute();
+  ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options);

aaron.ballman wrote:
> rsmith wrote:
> > This seems to be equivalent to setting `IncludeCXX11Attributes` to `true`, 
> > which seems to be equivalent to not setting it at all.
> Hmmm, not quite -- `AL.isCXX11Attribute()` may return `false` (like for the 
> GNU spelling of this attribute).
It might, but we only care about the value of `IncludeCXX11Attributes` when 
processing a C++11 attribute, so it doesn't matter how this flag is set for a 
non-C++11 attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for your work on this, I generally like this new approach and think 
we're heading in the right direction.




Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
   /// Attrs - Attributes.
   ParsedAttributes Attrs;
 
+  ParsedAttributes DeclarationAttrs;

We should probably rename `Attrs` to be less generic and add some comments to 
explain why there are two lists of parsed attributes.



Comment at: clang/include/clang/Sema/ParsedAttr.h:657-658
+  /// "slides" to the decl-specifier-seq).
+  /// Attributes with GNU, __declspec or keyword syntax generally slide
+  /// to the decl-specifier-seq. C++11 attributes specified ahead of the
+  /// declaration always appertain to the declaration according to the 
standard,

rsmith wrote:
> I don't think this sentence is correct: "Attributes with GNU, __declspec or 
> keyword syntax generally slide to the decl-specifier-seq." Instead, I think 
> those attribute syntaxes are never parsed as declaration attributes in the 
> first place, so there is no possibility of "sliding" anywhere -- they simply 
> always are decl-spec attributes.
That's fair -- I tend to think "sliding" is also what happens (at least 
notionally) in a case like `__attribute__((address_space(1))) int *i;` because 
it's a type attribute rather than a declaration attribute, but that's also a 
declaration specifier, so it doesn't really slide anywhere.



Comment at: clang/lib/Parse/ParseDecl.cpp:2188
 // Parse the next declarator.
-D.clear();
+D.clearExceptDeclarationAttrs();
 D.setCommaLoc(CommaLoc);

rsmith wrote:
> I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would 
> be better here -- something that indicates why we're clearing rather than 
> describing how.
Personally, I like `prepareForNextDeclarator()` -- that's nice and clear (to 
me).



Comment at: clang/lib/Parse/ParseDecl.cpp:4323-4326
+// C2x draft 6.7.2.1/9 : "The optional attribute specifier sequence in a
+// member declaration appertains to each of the members declared by the
+// member declarator list; it shall not appear if the optional member
+// declarator list is omitted."

Good catch! This also applies in C++: 
http://eel.is/c++draft/class#mem.general-14

I think you should add some test coverage for this, along the lines of:
```
struct S {
  [[clang::annotate("test")]] int; // The attribute should be diagnosed (as an 
error?)
};
```



Comment at: clang/lib/Parse/ParseDecl.cpp:6978-6997
 // Parse any C++11 attributes.
-MaybeParseCXX11Attributes(DS.getAttributes());
+ParsedAttributes ArgDeclAttrs(AttrFactory);
+MaybeParseCXX11Attributes(ArgDeclAttrs);
 
-// Skip any Microsoft attributes before a param.
-MaybeParseMicrosoftAttributes(DS.getAttributes());
-
-SourceLocation DSStart = Tok.getLocation();
+ParsedAttributes ArgDeclSpecAttrs(AttrFactory);
 
 // If the caller parsed attributes for the first argument, add them now.

rsmith wrote:
> Seems to be mostly pre-existing, but I don't think this is right. The 
> `FirstArgAttrs` are all decl-specifier attributes (they're parsed in 
> `ParseParenDeclarator`, where we look for GNU attributes and `__declspec` 
> attributes), so if we parsed any of those, we should not now parse any syntax 
> that is supposed to precede decl-specifier attributes. The current code 
> allows attributes to appear in the wrong order in the case where we need to 
> disambiguate a paren declarator: https://godbolt.org/z/bzK6n8obM (note that 
> the `g` case properly errors, but the `f` case that needs lookahead to 
> determine whether the `(` is introducing a function declarator incorrectly 
> accepts misplaced attributes).
Good catch!



Comment at: clang/lib/Parse/ParseObjc.cpp:1233-1234
   // Now actually move the attributes over.
   takeDeclAttributes(attrs, D.getMutableDeclSpec().getAttributes());
+  takeDeclAttributes(attrs, D.getDeclarationAttributes());
   takeDeclAttributes(attrs, D.getAttributes());

rsmith wrote:
> I think we should keep the attributes in appearance order.
+1

FWIW, we run into some "fun" bugs with attribute orders and declaration merging 
where the orders are opposite and it causes problems as in 
https://godbolt.org/z/58jTM4sGM (note how the error and the note swap 
positions), so the order of attributes tends to be important (both for semantic 
behavior as well as diagnostic behavior).



Comment at: clang/lib/Parse/ParseStmt.cpp:198
+  ParsedAttributes Attrs(AttrFactory);
+  ConcatenateAttributes(CXX11Attrs, GNUAttrs, Attrs);
+

I *think* this is going to be okay because attributes at the start of a 
statement have a very specific ordering, but one thing I was slightly worried 
about is attribute orders 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This direction looks good to me. Thanks!

Regarding the possibility of storing the declaration attributes on the 
`DeclSpec` rather than on the `Declarator`, I wonder if a better choice might 
be to make the `Declarator` hold a non-owning reference to the attributes, like 
it does for the `DeclSpec`?




Comment at: clang/include/clang/Sema/DeclSpec.h:1975-1984
   /// Reset the contents of this Declarator.
   void clear() {
+clearExceptDeclarationAttrs();
+DeclarationAttrs.clear();
+  }
+
+  /// Reset the contents of this Declarator, except for the declaration

Note that neither of these functions clear the `DeclSpec`. I suppose that makes 
sense given that the `Declarator` does not own the `DeclSpec` but merely holds 
a reference to it. Perhaps we could do the same thing with the declaration 
attributes?



Comment at: clang/include/clang/Sema/ParsedAttr.h:657-658
+  /// "slides" to the decl-specifier-seq).
+  /// Attributes with GNU, __declspec or keyword syntax generally slide
+  /// to the decl-specifier-seq. C++11 attributes specified ahead of the
+  /// declaration always appertain to the declaration according to the 
standard,

I don't think this sentence is correct: "Attributes with GNU, __declspec or 
keyword syntax generally slide to the decl-specifier-seq." Instead, I think 
those attribute syntaxes are never parsed as declaration attributes in the 
first place, so there is no possibility of "sliding" anywhere -- they simply 
always are decl-spec attributes.



Comment at: clang/lib/Parse/ParseDecl.cpp:1832
   // Parse the common declaration-specifiers piece.
   ParsingDeclSpec DS(*this);
 

Ideally I think we should pass the `DeclSpecAttrs` into `DS` up-front here. 
That'd keep the behavior closer to the behavior we get for attributes we parse 
in the `ParseDeclarationSpecifiers` call below, and will keep attributes in the 
correct order in the case of something like `__attribute__((a)) int 
__attribute__((b)) x;` at block scope.



Comment at: clang/lib/Parse/ParseDecl.cpp:2188
 // Parse the next declarator.
-D.clear();
+D.clearExceptDeclarationAttrs();
 D.setCommaLoc(CommaLoc);

I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would be 
better here -- something that indicates why we're clearing rather than 
describing how.



Comment at: clang/lib/Parse/ParseDecl.cpp:4385
+// can put them on the next field.
+Attrs.takeAllFrom(DeclaratorInfo.D.getDeclarationAttributes());
   }

I think we'd benefit here from the `Declarator` only holding a reference to the 
declaration attributes rather than owning them.



Comment at: clang/lib/Parse/ParseDecl.cpp:6978-6997
 // Parse any C++11 attributes.
-MaybeParseCXX11Attributes(DS.getAttributes());
+ParsedAttributes ArgDeclAttrs(AttrFactory);
+MaybeParseCXX11Attributes(ArgDeclAttrs);
 
-// Skip any Microsoft attributes before a param.
-MaybeParseMicrosoftAttributes(DS.getAttributes());
-
-SourceLocation DSStart = Tok.getLocation();
+ParsedAttributes ArgDeclSpecAttrs(AttrFactory);
 
 // If the caller parsed attributes for the first argument, add them now.

Seems to be mostly pre-existing, but I don't think this is right. The 
`FirstArgAttrs` are all decl-specifier attributes (they're parsed in 
`ParseParenDeclarator`, where we look for GNU attributes and `__declspec` 
attributes), so if we parsed any of those, we should not now parse any syntax 
that is supposed to precede decl-specifier attributes. The current code allows 
attributes to appear in the wrong order in the case where we need to 
disambiguate a paren declarator: https://godbolt.org/z/bzK6n8obM (note that the 
`g` case properly errors, but the `f` case that needs lookahead to determine 
whether the `(` is introducing a function declarator incorrectly accepts 
misplaced attributes).



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2716
 // Otherwise, it must be a using-declaration or an alias-declaration.
 return ParseUsingDeclaration(DeclaratorContext::Member, TemplateInfo,
+ UsingLoc, DeclEnd, DeclAttrs, AS);

Do we need to `ProhibitAttrs(DeclSpecAttrs)` along this code path? For example:
```
struct A { int a; };
struct B : A { [uuid("1234")] using A::a; };
```
... currently warns (for `-target x86_64-windows`) but I think with this patch 
we'll silently drop the `uuid` attribute on the floor.



Comment at: clang/lib/Parse/ParseObjc.cpp:661
 allTUVariables.push_back(
-ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs));
+ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs, attrs));
 continue;

It's a bit confusing to use the same `attrs` 

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme created this revision.
Herald added a reviewer: aaron.ballman.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
mboehme requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

(Iteration #4)

For backwards compatiblity, we emit only a warning instead of an error if the
attribute is one of the existing type attributes that we have historically
allowed to "slide" to the `DeclSpec` just as if it had been specified in GNU
syntax. (We will call these "legacy type attributes" below.)

The high-level changes that achieve this are:

- We introduce a new field `Declarator::DeclarationAttrs` (with appropriate 
accessors) to store C++11 attributes occurring in the attribute-specifier-seq 
at the beginning of a simple-declaration (and other similar declarations). 
Previously, these attributes were placed on the `DeclSpec`, which made it 
impossible to reconstruct later on whether the attributes had in fact been 
placed on the decl-specifier-seq or ahead of the declaration.

- In the parser, we propgate declaration attributes and decl-specifier-seq 
attributes separately until we can place them in `Declarator::DeclarationAttrs` 
or `DeclSpec::Attrs`, respectively.

- In `ProcessDeclAttributes()`, in addition to processing declarator 
attributes, we now also process the attributes from 
`Declarator::DeclarationAttrs` (except if they are legacy type attributes).

- In `ConvertDeclSpecToType()`, in addition to processing `DeclSpec` 
attributes, we also process any legacy type attributes that occur in 
`Declarator::DeclarationAttrs` (and emit a warning).

- We make `ProcessDeclAttribute` emit an error if it sees any non-declaration 
attributes in C++11 syntax, except in the following cases:
  - If it is being called for attributes on a `DeclSpec` or `DeclaratorChunk`
  - If the attribute is a legacy type attribute (in which case we only emit a 
warning)

The standard justifies treating attributes at the beginning of a
simple-declaration and attributes after a declarator-id the same. Here are some
relevant parts of the standard:

- The attribute-specifier-seq at the beginning of a simple-declaration 
"appertains to each of the entities declared by the declarators of the 
init-declarator-list" (https://eel.is/c++draft/dcl.dcl#dcl.pre-3)

- "In the declaration for an entity, attributes appertaining to that entity can 
appear at the start of the declaration and after the declarator-id for that 
declaration." (https://eel.is/c++draft/dcl.dcl#dcl.pre-note-2)

- "The optional attribute-specifier-seq following a declarator-id appertains to 
the entity that is declared." 
(https://eel.is/c++draft/dcl.dcl#dcl.meaning.general-1)

The standard contains similar wording to that for a simple-declaration in other
similar types of declarations, for example:

- "The optional attribute-specifier-seq in a parameter-declaration appertains 
to the parameter." (https://eel.is/c++draft/dcl.fct#3)

- "The optional attribute-specifier-seq in an exception-declaration appertains 
to the parameter of the catch clause" (https://eel.is/c++draft/except.pre#1)

The new behavior is tested both on the newly added type attribute
`annotate_type`, for which we emit errors, and for the legacy type attribute
`address_space` (chosen somewhat randomly from the various legacy type
attributes), for which we emit warnings.

Depends On D111548 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126061

Files:
  clang/include/clang/Basic/AttributeCommonInfo.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Attributes.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseObjc.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/ParsedAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/address-space-placement.cpp
  clang/test/SemaCXX/annotate-type.cpp
  clang/test/SemaOpenCL/address-spaces.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -3734,7 +3734,7 @@
 if (!StmtSubjects.empty()) {
   OS << "bool diagAppertainsToDecl(Sema , const ParsedAttr , ";
   OS << "const Decl *D) const override {\n";
-  OS << "  S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)\n";
+  OS << "  S.Diag(AL.getLoc(),