Skip to content

[Clang] Allow parsing arbitrary order of attributes for declarations #133107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
May 9, 2025
Merged

[Clang] Allow parsing arbitrary order of attributes for declarations #133107

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c46eda6
[CLANG] Enable alignas after GNU attributes
Mar 26, 2025
517ba9e
[CLANG] Enable cxx11 attributes after gnu attributes in ParseCXXClass…
Mar 28, 2025
2b06658
[CLANG] Move microsoft attrs into loop
Apr 4, 2025
04415ee
[CLANG] Arbitrary attribute order
Apr 8, 2025
7388c93
Merge branch 'main' into alignas-order
Apr 8, 2025
4d0a4f0
Style fix
Apr 8, 2025
e517ff6
[CLANG] Added more tests, removed range assert
Apr 9, 2025
ebd962d
Merge branch 'main' into alignas-order
DenisGZM Apr 15, 2025
453f5b2
review fixes
Apr 16, 2025
c5bc577
Merge remote-tracking branch 'upstream/main' into alignas-order
Apr 19, 2025
5c8ec3c
[CLANG] Set DeclStart at start of parsed attributes
Apr 26, 2025
a693469
Merge branch 'main' into alignas-order
Apr 26, 2025
0223b3e
Merge branch 'main' into alignas-order
DenisGZM May 6, 2025
121b800
Merge branch 'main' into alignas-order
DenisGZM May 9, 2025
d0df126
Merge branch 'main' into alignas-order
DenisGZM May 9, 2025
4922d9e
Merge branch 'main' into alignas-order
AaronBallman May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ Bug Fixes to C++ Support
- Clang no longer crashes when establishing subsumption between some constraint expressions. (#GH122581)
- Clang now issues an error when placement new is used to modify a const-qualified variable
in a ``constexpr`` function. (#GH131432)
- Clang now correctly parses arbitrary order of ``[[]]``, ``__attribute__`` and ``alignas`` attributes for declarations (#GH133107)
- Clang now emits a warning when class template argument deduction for alias templates is used in C++17. (#GH133806)

Bug Fixes to AST Handling
Expand Down
5 changes: 4 additions & 1 deletion clang/include/clang/Parse/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3062,13 +3062,16 @@ class Parser : public CodeCompletionHandler {
bool CouldBeBitField = false);
Decl *ParseHLSLBuffer(SourceLocation &DeclEnd);

void MaybeParseMicrosoftAttributes(ParsedAttributes &Attrs) {
bool MaybeParseMicrosoftAttributes(ParsedAttributes &Attrs) {
bool AttrsParsed = false;
if ((getLangOpts().MicrosoftExt || getLangOpts().HLSL) &&
Tok.is(tok::l_square)) {
ParsedAttributes AttrsWithRange(AttrFactory);
ParseMicrosoftAttributes(AttrsWithRange);
AttrsParsed = AttrsWithRange.size();
Attrs.takeAllFrom(AttrsWithRange);
}
return AttrsParsed;
}
void ParseMicrosoftUuidAttributeArgs(ParsedAttributes &Attrs);
void ParseMicrosoftAttributes(ParsedAttributes &Attrs);
Expand Down
7 changes: 5 additions & 2 deletions clang/lib/Parse/ParseDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,11 +3035,14 @@ Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclaration(
}

ParsedAttributes DeclSpecAttrs(AttrFactory);
MaybeParseMicrosoftAttributes(DeclSpecAttrs);

// Hold late-parsed attributes so we can attach a Decl to them later.
LateParsedAttrList CommonLateParsedAttrs;

while (MaybeParseCXX11Attributes(DeclAttrs) ||
MaybeParseGNUAttributes(DeclSpecAttrs, &CommonLateParsedAttrs) ||
MaybeParseMicrosoftAttributes(DeclSpecAttrs))
;

// decl-specifier-seq:
// Parse the common declaration-specifiers piece.
ParsingDeclSpec DS(*this, TemplateDiags);
Expand Down
13 changes: 8 additions & 5 deletions clang/lib/Parse/ParseStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,9 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
}

default: {
if (getLangOpts().CPlusPlus && MaybeParseCXX11Attributes(CXX11Attrs, true))
goto Retry;

bool HaveAttrs = !CXX11Attrs.empty() || !GNUAttrs.empty();
auto IsStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); };
bool AllAttrsAreStmtAttrs = llvm::all_of(CXX11Attrs, IsStmtAttr) &&
Expand All @@ -260,11 +263,11 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
GNUAttrs);
}
if (CXX11Attrs.Range.getBegin().isValid()) {
// The caller must guarantee that the CXX11Attrs appear before the
// GNUAttrs, and we rely on that here.
assert(GNUAttrs.Range.getBegin().isInvalid() ||
GNUAttrs.Range.getBegin() > CXX11Attrs.Range.getBegin());
DeclStart = CXX11Attrs.Range.getBegin();
// Order of C++11 and GNU attributes is may be arbitrary.
DeclStart = GNUAttrs.Range.getBegin().isInvalid()
? CXX11Attrs.Range.getBegin()
: std::min(CXX11Attrs.Range.getBegin(),
GNUAttrs.Range.getBegin());
} else if (GNUAttrs.Range.getBegin().isValid())
DeclStart = GNUAttrs.Range.getBegin();
return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ast-dump-color.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ struct Invalid {
//CHECK: {{^}}[[Blue]]| `-[[RESET]][[MAGENTA]]DeclRefExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:40[[RESET]]> [[Green]]'class Mutex':'Mutex'[[RESET]][[Cyan]] lvalue[[RESET]][[Cyan]][[RESET]] [[GREEN]]Var[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]][[CYAN]] 'mu1'[[RESET]] [[Green]]'class Mutex':'Mutex'[[RESET]] non_odr_use_unevaluated{{$}}
//CHECK: {{^}}[[Blue]]|-[[RESET]][[GREEN]]CXXRecordDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:28:1[[RESET]], [[Yellow]]line:30:1[[RESET]]> [[Yellow]]line:28:8[[RESET]] struct[[CYAN]] Invalid[[RESET]] definition
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXRecordDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:1[[RESET]], [[Yellow]]col:8[[RESET]]> [[Yellow]]col:8[[RESET]] implicit referenced struct[[CYAN]] Invalid[[RESET]]
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXConstructorDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:29:3[[RESET]], [[Yellow]]col:42[[RESET]]> [[Yellow]]col:29[[RESET]] invalid[[CYAN]] Invalid[[RESET]] [[Green]]'void (int)'[[RESET]]
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXConstructorDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:29:29[[RESET]], [[Yellow]]col:42[[RESET]]> [[Yellow]]col:29[[RESET]] invalid[[CYAN]] Invalid[[RESET]] [[Green]]'void (int)'[[RESET]]
//CHECK: {{^}}[[Blue]]| | |-[[RESET]][[GREEN]]ParmVarDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:37[[RESET]], [[Yellow]]<invalid sloc>[[RESET]]> [[Yellow]]col:42[[RESET]] invalid [[Green]]'int'[[RESET]]
//CHECK: {{^}}[[Blue]]| | `-[[RESET]][[BLUE]]NoInlineAttr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:18[[RESET]]>
//CHECK: {{^}}[[Blue]]| |-[[RESET]][[GREEN]]CXXConstructorDecl[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:28:8[[RESET]]> [[Yellow]]col:8[[RESET]] implicit used constexpr[[CYAN]] Invalid[[RESET]] [[Green]]'void () noexcept'[[RESET]] inline
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Index/annotate-attribute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ int templateFunction(T value) __attribute__((annotate("works")));

// CHECK: ClassDecl=Test:3:7 (Definition) Extent=[3:1 - 17:2]
// CHECK-NEXT: CXXAccessSpecifier=:4:1 (Definition) Extent=[4:1 - 4:8]
// CHECK-NEXT: CXXMethod=aMethod:5:51 Extent=[5:3 - 5:60]
// CHECK-NEXT: CXXMethod=aMethod:5:51 Extent=[5:46 - 5:60]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we went from pointing to the start of __attribute__ to pointing to the start of void which is a bit unfortunate.

Copy link
Contributor Author

@DenisGZM DenisGZM Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should avoid it somehow? Or just accept it as is?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence. It's not the worst regression in behavior, but it does make the diagnostic slightly harder for users to reason about. WDYT @erichkeane ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its really quite unfortunate... I think it is at least worth seeing how much work needs to be done to get this 'right', and see if it is worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem here is to determine 'right' :)
Now I set annotated declaration range at the begining of the first parsed attribute and it might be not only DeclSpecAttr.
Earlier we parsed only CXX attrs before ParseDeclarationSpecifiers and then annotated range could only contain DeclSpecAttrs in it.

Examples:

class Test {
public:
  __attribute__((annotate("spiffy_method"))) [[deprecated]] void aMethod();  // Error before, now: Extent=[5:3 - 5:75]
};

class Test {
public:
  [[deprecated]] __attribute__((annotate("spiffy_method")))  void aMethod(); // Before: Extent=[5:18 - 5:75], now: Extent=[5:3 - 5:75]
};

Is it what expected to be done?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the underlying issue is that we're using in-band information about source ranges that's no longer true. We used to be able to rely on the source range because the order was more strict, but as we've relaxed it, you can now mix declaration and decl specifier attributes in more exotic ways.

However, addressing that may be quite involved. So I think we should probably accept this as-is; pointing to the start of the list is better than pointing to the type.

// CHECK-NEXT: attribute(annotate)=spiffy_method Extent=[5:18 - 5:43]
// CHECK-NEXT: CXXAccessSpecifier=:7:1 (Definition) Extent=[7:1 - 7:43]
// CHECK-NEXT: attribute(annotate)=works Extent=[7:23 - 7:40]
Expand Down
23 changes: 23 additions & 0 deletions clang/test/Parser/c2x-alignas.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,26 @@

_Alignas(int) struct c1; // expected-warning {{'_Alignas' attribute ignored}}
alignas(int) struct c1; // expected-warning {{'alignas' attribute ignored}}


__attribute__(()) [[]] alignas(int) int a; // expected-none TODO: actually this line should be an error
__attribute__(()) alignas(int) [[]] int b; // expected-error {{an attribute list cannot appear here}}
__attribute__(()) alignas(int) int c; // expected-none
[[]] __attribute__(()) alignas(int) int d; // expected-none
alignas(int) [[]] __attribute__(()) int e; // expected-error {{an attribute list cannot appear here}}

struct C1 {
__attribute__(()) [[]] alignas(int) int a; // expected-error {{an attribute list cannot appear here}}
__attribute__(()) alignas(int) [[]] int b; // expected-error {{an attribute list cannot appear here}}
__attribute__(()) alignas(int) int c; // expected-none
[[]] __attribute__(()) alignas(int) int d; // expected-none
alignas(int) [[]] __attribute__(()) int e; // expected-error {{an attribute list cannot appear here}}
};

void fn_with_decl() {
__attribute__(()) [[]] alignas(int) int a; // expected-error {{an attribute list cannot appear here}}
__attribute__(()) alignas(int) [[]] int b; // expected-error {{an attribute list cannot appear here}}
__attribute__(()) alignas(int) int c; // expected-none
[[]] __attribute__(()) alignas(int) int d; // expected-none
alignas(int) [[]] __attribute__(()) int e; // expected-error {{an attribute list cannot appear here}}
}
24 changes: 24 additions & 0 deletions clang/test/Parser/cxx0x-attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,30 @@ namespace test_misplacement {
[[]] enum E2 { }; //expected-error{{misplaced attributes}}
}

__attribute__(()) alignas(int) int xx; // expected-none
__attribute__(()) alignas(int) [[]] int yy; // expected-none
[[]] __attribute__(()) alignas(int) int zz; // expected-none
alignas(int) [[]] __attribute__(()) int aa; // expected-none
[[]] alignas(int) __attribute__(()) int bb; // expected-none
__attribute__(()) [[]] alignas(int) int cc; // expected-none

class C1 {
__attribute__(()) alignas(int) int x; // expected-none
__attribute__(()) alignas(int) [[]] int y; // expected-none
[[]] __attribute__(()) alignas(int) int z; // expected-none
alignas(int) [[]] __attribute__(()) int a; // expected-none
[[]] alignas(int) __attribute__(()) int b; // expected-none
__attribute__(()) [[]] alignas(int) int c; // expected-none
};

void fn_with_decl() {
__attribute__(()) alignas(int) int x; // expected-none
__attribute__(()) alignas(int) [[]] int y; // expected-none
[[]] __attribute__(()) alignas(int) int z; // expected-none
alignas(int) [[]] __attribute__(()) int a; // expected-none
[[]] alignas(int) __attribute__(()) int b; // expected-none
__attribute__(()) [[]] alignas(int) int c; // expected-none
}
// Checks attributes placed at wrong syntactic locations of class specifiers.
class [[]] [[]]
attr_after_class_name_decl [[]] [[]]; // expected-error {{an attribute list cannot appear here}}
Expand Down
15 changes: 15 additions & 0 deletions clang/test/SemaCUDA/cuda-attr-order.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Verify that we can parse a simple CUDA file with different attributes order.
// RUN: %clang_cc1 "-triple" "nvptx-nvidia-cuda" -fsyntax-only -verify %s
// expected-no-diagnostics
#include "Inputs/cuda.h"

struct alignas(16) float4 {
float x, y, z, w;
};

__attribute__((device)) float func() {
__shared__ alignas(alignof(float4)) float As[4][4]; // Both combinations
alignas(alignof(float4)) __shared__ float Bs[4][4]; // must be legal

return As[0][0] + Bs[0][0];
}
8 changes: 4 additions & 4 deletions clang/test/SemaCXX/warn-thread-safety-analysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2434,10 +2434,10 @@ class Foo {
namespace WarnNoDecl {

class Foo {
void foo(int a); __attribute__(( // \
// expected-warning {{declaration does not declare anything}}
exclusive_locks_required(a))); // \
// expected-warning {{attribute exclusive_locks_required ignored}}
void foo(int a); __attribute__((
exclusive_locks_required(a))); // expected-warning {{declaration does not declare anything}} \
// expected-warning {{attribute exclusive_locks_required ignored}}

};

} // end namespace WarnNoDecl
Expand Down
Loading