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 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c46eda6
[CLANG] Enable alignas after GNU attributes
DenisGZM Mar 26, 2025
517ba9e
[CLANG] Enable cxx11 attributes after gnu attributes in ParseCXXClass…
DenisGZM Mar 28, 2025
2b06658
[CLANG] Move microsoft attrs into loop
DenisGZM Apr 4, 2025
04415ee
[CLANG] Arbitrary attribute order
DenisGZM Apr 8, 2025
7388c93
Merge branch 'main' into alignas-order
DenisGZM Apr 8, 2025
4d0a4f0
Style fix
DenisGZM Apr 8, 2025
e517ff6
[CLANG] Added more tests, removed range assert
DenisGZM Apr 9, 2025
ebd962d
Merge branch 'main' into alignas-order
DenisGZM Apr 15, 2025
453f5b2
review fixes
DenisGZM Apr 16, 2025
c5bc577
Merge remote-tracking branch 'upstream/main' into alignas-order
DenisGZM Apr 19, 2025
5c8ec3c
[CLANG] Set DeclStart at start of parsed attributes
DenisGZM Apr 26, 2025
a693469
Merge branch 'main' into alignas-order
DenisGZM 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
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,10 +3035,13 @@ 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.
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/Parse/ParseStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,11 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
goto Retry;
}

case tok::kw_alignas: {
ParseAlignmentSpecifier(CXX11Attrs);
goto Retry;
}

case tok::kw_template: {
SourceLocation DeclEnd;
ParseTemplateDeclarationOrSpecialization(DeclaratorContext::Block, 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

@AaronBallman AaronBallman Apr 15, 2025

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

@AaronBallman AaronBallman Apr 21, 2025

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

@erichkeane erichkeane Apr 21, 2025

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

@DenisGZM DenisGZM Apr 26, 2025

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

@AaronBallman AaronBallman May 1, 2025

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
6 changes: 6 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 @@ -71,6 +71,12 @@ namespace test_misplacement {
[[]] union union_attr2; //expected-error{{misplaced attributes}}
[[]] enum E2 { }; //expected-error{{misplaced attributes}}
}
struct S1 { __attribute__((deprecated)) alignas(16) int x; }; // expected-none
class C1 { __attribute__((deprecated)) alignas(16) int x; }; // expected-none

void fn_with_decl() {
__attribute__((deprecated)) alignas(16) int x; // expected-none
}

// Checks attributes placed at wrong syntactic locations of class specifiers.
class [[]] [[]]
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