[Clang] Fix: Restore warning inadvertently removed by D126061.

[mboehme]

Summary

Before D126061, Clang would warn about this code

struct X {
    [[deprecated]] struct Y {};
};

with the warning

attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration

D126061 inadvertently caused this warning to no longer be emitted. This patch
restores the previous behavior.

The reason for the bug is that after D126061, C++11 attributes applied to a
member declaration are no longer placed in DS.getAttributes() but are instead
tracked in a separate list (DeclAttrs). In the case of a free-standing
decl-specifier-seq, we would simply ignore the contents of this list. Instead,
we now pass the list on to Sema::ParsedFreeStandingDeclSpec() so that it can
issue the appropriate warning.

[aaron.ballman]

Thank you for the quick fix! I had a question about why only one of the two interfaces needed to be updated and whether we thought that was sensible, but otherwise the changes here look correct to me.

clang/lib/Sema/SemaDecl.cpp

4631–4637

It's surprising that we don't need to update this interface as well; is that inconsistency desired?

[mboehme]

Changes in response to review comments.

[mboehme]

clang/lib/Sema/SemaDecl.cpp

4631–4637

This is OK because every caller of this overload happens to do ProhibitAttributes(DeclAttrs).

But now that you bring this up, I realize that this is risky. What if we add another caller in the future that doesn’t do ProhibitAttribubtes(DeclAttrs)? And even in the current situation, I think it makes sense to be explicit that we’re passing an empty list of attributes.

So I’ve added the DeclAttrs parameter to this function too, and now pass ParsedAttributesView::none() at the callsites.

[mboehme]

Turn off clang-format for attr-declspec-ignored.cpp.

It has existing formatting (indentation within namespaces) that's incompatible
with clang-format.

[aaron.ballman]

LGTM aside from a minor nit in the testing.

clang/lib/Sema/SemaDecl.cpp

4631–4637

Thanks, I think this is more clear.

clang/test/SemaCXX/attr-declspec-ignored.cpp

3–6

We don't typically put clang-format markings into tests -- we don't require that tests be formatted, and it's a bug that precommit CI continues to fail because of it (https://github.com/llvm/llvm-project/issues/55982).

Feel free to remove this bit when landing.

62

Same here for this one.

[mboehme]

Remove clang-format directives from test.

[mboehme]

clang/test/SemaCXX/attr-declspec-ignored.cpp

3–6

Got it. Done!

I've also commented on the bug.

62

Done.

[mboehme]

Remove trailing spaces

[miyuki]

Thanks for fixing the issue.

clang/include/clang/Sema/Sema.h

public:
   static PrintingPolicy getPrintingPolicy(const ASTContext &Ctx,
                                           const Preprocessor &PP);
 
   /// Scope actions.
   void ActOnPopScope(SourceLocation Loc, Scope *S);
   void ActOnTranslationUnitScope(Scope *S);
 
   Decl *ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,
+                                   const ParsedAttributesView &DeclAttrs,
                                    RecordDecl *&AnonRecord);
   Decl *ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,
+                                   const ParsedAttributesView &DeclAttrs,
                                    MultiTemplateParamsArg TemplateParams,
                                    bool IsExplicitInstantiation,
                                    RecordDecl *&AnonRecord);
 
   Decl *BuildAnonymousStructOrUnion(Scope *S, DeclSpec &DS,
                                     AccessSpecifier AS,
                                     RecordDecl *Record,
                                     const PrintingPolicy &Policy);

clang/lib/Parse/ParseDecl.cpp

Parser::DeclGroupPtrTy Parser::ParseSimpleDeclaration(
 
   // C99 6.7.2.3p6: Handle "struct-or-union identifier;", "enum { X };"
   // declaration-specifiers init-declarator-list[opt] ';'
   if (Tok.is(tok::semi)) {
     ProhibitAttributes(DeclAttrs);
     DeclEnd = Tok.getLocation();
     if (RequireSemi) ConsumeToken();
     RecordDecl *AnonRecord = nullptr;
-    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(getCurScope(), AS_none,
-                                                       DS, AnonRecord);
+    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(
+        getCurScope(), AS_none, DS, ParsedAttributesView::none(), AnonRecord);
     DS.complete(TheDecl);
     if (AnonRecord) {
       Decl* decls[] = {AnonRecord, TheDecl};
       return Actions.BuildDeclaratorGroup(decls);
     }
     return Actions.ConvertDeclToDeclGroup(TheDecl);
   }
 
void Parser::ParseStructDeclaration(
   // specifier. Let the actions module cope with it.
   if (Tok.is(tok::semi)) {
     // C2x 6.7.2.1p9 : "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."
     ProhibitAttributes(Attrs);
     RecordDecl *AnonRecord = nullptr;
-    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(getCurScope(), AS_none,
-                                                       DS, AnonRecord);
+    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(
+        getCurScope(), AS_none, DS, ParsedAttributesView::none(), AnonRecord);
     assert(!AnonRecord && "Did not expect anonymous struct or union here");
     DS.complete(TheDecl);
     return;
   }
 
   // Read struct-declarators until we find the semicolon.
   bool FirstDeclarator = true;
   SourceLocation CommaLoc;

clang/lib/Parse/ParseDeclCXX.cpp

MultiTemplateParamsArg TemplateParams(
       TemplateInfo.TemplateParams? TemplateInfo.TemplateParams->size() : 0);
 
   if (TryConsumeToken(tok::semi)) {
     if (DS.isFriendSpecified())
       ProhibitAttributes(DeclAttrs);
 
     RecordDecl *AnonRecord = nullptr;
     Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(
-        getCurScope(), AS, DS, TemplateParams, false, AnonRecord);
+        getCurScope(), AS, DS, DeclAttrs, TemplateParams, false, AnonRecord);
     DS.complete(TheDecl);
     if (AnonRecord) {
       Decl* decls[] = {AnonRecord, TheDecl};
       return Actions.BuildDeclaratorGroup(decls);
     }
     return Actions.ConvertDeclToDeclGroup(TheDecl);
   }
 

clang/lib/Parse/ParseTemplate.cpp

Decl *Parser::ParseSingleDeclarationAfterTemplate(
   ParseDeclarationSpecifiers(DS, TemplateInfo, AS,
                              getDeclSpecContextFromDeclaratorContext(Context));
 
   if (Tok.is(tok::semi)) {
     ProhibitAttributes(prefixAttrs);
     DeclEnd = ConsumeToken();
     RecordDecl *AnonRecord = nullptr;
     Decl *Decl = Actions.ParsedFreeStandingDeclSpec(
-        getCurScope(), AS, DS,
+        getCurScope(), AS, DS, ParsedAttributesView::none(),
         TemplateInfo.TemplateParams ? *TemplateInfo.TemplateParams
                                     : MultiTemplateParamsArg(),
         TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation,
         AnonRecord);
     assert(!AnonRecord &&
            "Anonymous unions/structs should not be valid with template");
     DS.complete(Decl);
     return Decl;

clang/lib/Parse/Parser.cpp

if (Tok.is(tok::semi)) {
     SourceLocation CorrectLocationForAttributes =
         DeclSpec::isDeclRep(DS.getTypeSpecType())
             ? DS.getTypeSpecTypeLoc().getLocWithOffset(
                   LengthOfTSTToken(DS.getTypeSpecType()))
             : SourceLocation();
     ProhibitAttributes(Attrs, CorrectLocationForAttributes);
     ConsumeToken();
     RecordDecl *AnonRecord = nullptr;
-    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(getCurScope(), AS_none,
-                                                       DS, AnonRecord);
+    Decl *TheDecl = Actions.ParsedFreeStandingDeclSpec(
+        getCurScope(), AS_none, DS, ParsedAttributesView::none(), AnonRecord);
     DS.complete(TheDecl);
     if (AnonRecord) {
       Decl* decls[] = {AnonRecord, TheDecl};
       return Actions.BuildDeclaratorGroup(decls);
     }
     return Actions.ConvertDeclToDeclGroup(TheDecl);
   }
 

clang/lib/Sema/SemaDecl.cpp

if (!hasVisibleDefinition(Old) &&
     Diag(New->getLocation(), diag::err_redefinition) << New;
     notePreviousDefinition(Old, New->getLocation());
     New->setInvalidDecl();
     return true;
   }
 }
 
 /// ParsedFreeStandingDeclSpec - This method is invoked when a declspec with
 /// no declarator (e.g. "struct foo;") is parsed.
-Decl *
-Sema::ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,
+Decl *Sema::ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS,
+                                       DeclSpec &DS,
+                                       const ParsedAttributesView &DeclAttrs,
                                        RecordDecl *&AnonRecord) {
-  return ParsedFreeStandingDeclSpec(S, AS, DS, MultiTemplateParamsArg(), false,
-                                    AnonRecord);
+  return ParsedFreeStandingDeclSpec(
+      S, AS, DS, DeclAttrs, MultiTemplateParamsArg(), false, AnonRecord);
 }
 
 // The MS ABI changed between VS2013 and VS2015 with regard to numbers used to
 // disambiguate entities defined in different scopes.
 // While the VS2015 ABI fixes potential miscompiles, it is also breaks
 // compatibility.
 // We will pick our mangling number depending on which version of MSVC is being
 // targeted.
static unsigned GetDiagnosticTypeSpecifierID(DeclSpec::TST T) {
   default:
     llvm_unreachable("unexpected type specifier");
   }
 }
 
 /// ParsedFreeStandingDeclSpec - This method is invoked when a declspec with
 /// no declarator (e.g. "struct foo;") is parsed. It also accepts template
 /// parameters to cope with template friend declarations.
-Decl *
-Sema::ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS, DeclSpec &DS,
+Decl *Sema::ParsedFreeStandingDeclSpec(Scope *S, AccessSpecifier AS,
+                                       DeclSpec &DS,
+                                       const ParsedAttributesView &DeclAttrs,
                                        MultiTemplateParamsArg TemplateParams,
                                        bool IsExplicitInstantiation,
                                        RecordDecl *&AnonRecord) {
   Decl *TagD = nullptr;
   TagDecl *Tag = nullptr;
   if (DS.getTypeSpecType() == DeclSpec::TST_class ||
       DS.getTypeSpecType() == DeclSpec::TST_struct ||
       DS.getTypeSpecType() == DeclSpec::TST_interface ||
       DS.getTypeSpecType() == DeclSpec::TST_union ||
       DS.getTypeSpecType() == DeclSpec::TST_enum) {
     TagD = DS.getRepAsDecl();
if (DS.getTypeQualifiers() & DeclSpec::TQ_atomic)
       Diag(DS.getAtomicSpecLoc(), DiagID) << "_Atomic";
     if (DS.getTypeQualifiers() & DeclSpec::TQ_unaligned)
       Diag(DS.getUnalignedSpecLoc(), DiagID) << "__unaligned";
   }
 
   // Warn about ignored type attributes, for example:
   // __attribute__((aligned)) struct A;
   // Attributes should be placed after tag to apply to type declaration.
-  if (!DS.getAttributes().empty()) {
+  if (!DS.getAttributes().empty() || !DeclAttrs.empty()) {
     DeclSpec::TST TypeSpecType = DS.getTypeSpecType();
     if (TypeSpecType == DeclSpec::TST_class ||
         TypeSpecType == DeclSpec::TST_struct ||
         TypeSpecType == DeclSpec::TST_interface ||
         TypeSpecType == DeclSpec::TST_union ||
         TypeSpecType == DeclSpec::TST_enum) {
       for (const ParsedAttr &AL : DS.getAttributes())
         Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
             << AL << GetDiagnosticTypeSpecifierID(TypeSpecType);
+      for (const ParsedAttr &AL : DeclAttrs)
+        Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
+            << AL << GetDiagnosticTypeSpecifierID(TypeSpecType);
     }
   }
 
   return TagD;
 }
 
 /// We are trying to inject an anonymous member into the given scope;
 /// check if there's an existing declaration that can't be overloaded.

clang/test/SemaCXX/attr-declspec-ignored.cpp

 // RUN: %clang_cc1 %s -verify -fsyntax-only
 
+// For this test, we want the contents of the namespaces to be indented, which
+// clang-format doesn't like. Also, clang-format reformats the long lines in a
+// way that's difficult to read.
+// clang-format off
+
 namespace test1 {
   __attribute__((visibility("hidden")))  __attribute__((aligned)) class A; // expected-warning{{attribute 'visibility' is ignored, place it after "class" to apply attribute to type declaration}} \
   // expected-warning{{attribute 'aligned' is ignored, place it after "class" to apply attribute to type declaration}}
   __attribute__((visibility("hidden")))  __attribute__((aligned)) struct B; // expected-warning{{attribute 'visibility' is ignored, place it after "struct" to apply attribute to type declaration}} \
   // expected-warning{{attribute 'aligned' is ignored, place it after "struct" to apply attribute to type declaration}}
   __attribute__((visibility("hidden")))  __attribute__((aligned)) union C; // expected-warning{{attribute 'visibility' is ignored, place it after "union" to apply attribute to type declaration}} \
   // expected-warning{{attribute 'aligned' is ignored, place it after "union" to apply attribute to type declaration}} 
   __attribute__((visibility("hidden")))  __attribute__((aligned)) enum D {D}; // expected-warning{{attribute 'visibility' is ignored, place it after "enum" to apply attribute to type declaration}} \
   // expected-warning{{attribute 'aligned' is ignored, place it after "enum" to apply attribute to type declaration}}
+
+  // Test that we get the same warnings for type declarations nested in a record.
+  struct X {
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) class A; // expected-warning{{attribute 'visibility' is ignored, place it after "class" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "class" to apply attribute to type declaration}}
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) struct B; // expected-warning{{attribute 'visibility' is ignored, place it after "struct" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "struct" to apply attribute to type declaration}}
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) union C; // expected-warning{{attribute 'visibility' is ignored, place it after "union" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "union" to apply attribute to type declaration}} 
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) enum D {D}; // expected-warning{{attribute 'visibility' is ignored, place it after "enum" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "enum" to apply attribute to type declaration}}
+
+    // Also test [[]] attribute syntax. (On a non-nested declaration, these
+    // generate a hard "misplaced attributes" error, which we test for
+    // elsewhere.)
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] class E; // expected-warning{{attribute 'visibility' is ignored, place it after "class" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "class" to apply attribute to type declaration}}
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] struct F; // expected-warning{{attribute 'visibility' is ignored, place it after "struct" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "struct" to apply attribute to type declaration}}
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] union G; // expected-warning{{attribute 'visibility' is ignored, place it after "union" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "union" to apply attribute to type declaration}} 
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] enum H {H}; // expected-warning{{attribute 'visibility' is ignored, place it after "enum" to apply attribute to type declaration}} \
+    // expected-warning{{attribute 'aligned' is ignored, place it after "enum" to apply attribute to type declaration}}
+  };
 }
 
 namespace test2 {
   __attribute__((visibility("hidden")))  __attribute__((aligned)) class A {} a;
   __attribute__((visibility("hidden")))  __attribute__((aligned)) struct B {} b;
   __attribute__((visibility("hidden")))  __attribute__((aligned)) union C {} c;
   __attribute__((visibility("hidden")))  __attribute__((aligned)) enum D {D} d;
+
+  struct X {
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) class A {} a;
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) struct B {} b;
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) union C {} c;
+    __attribute__((visibility("hidden")))  __attribute__((aligned)) enum D {D} d;
+
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] class E {} e;
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] struct F {} f;
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] union G {} g;
+    [[gnu::visibility("hidden")]]  [[gnu::aligned]] enum H {H} h;
+  };
 }
+
+// clang-format on