Skip to content
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.

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

[CLANG] Enable alignas after GNU attributes #133107

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DenisGZM
Copy link
Contributor

Enable parsing alignas attribute after GNU attributes, before ParseDeclaration

This might be useful for cuda code where shared and other specificators may be mixed with align.

I'd be glad to see if there are any better places or other technique to process this attribute without interrupting current flow of parsing.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 26, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 26, 2025

@llvm/pr-subscribers-clang

Author: Denis.G (DenisGZM)

Changes

Enable parsing alignas attribute after GNU attributes, before ParseDeclaration

This might be useful for cuda code where shared and other specificators may be mixed with align.

I'd be glad to see if there are any better places or other technique to process this attribute without interrupting current flow of parsing.


Full diff: https://github.com/llvm/llvm-project/pull/133107.diff

2 Files Affected:

  • (modified) clang/lib/Parse/ParseStmt.cpp (+5)
  • (added) clang/test/SemaCUDA/cuda-attr-order.cu (+15)
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 150b2879fc94f..33b9f63bcfa08 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -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,
diff --git a/clang/test/SemaCUDA/cuda-attr-order.cu b/clang/test/SemaCUDA/cuda-attr-order.cu
new file mode 100644
index 0000000000000..d3bf5b014d1c6
--- /dev/null
+++ b/clang/test/SemaCUDA/cuda-attr-order.cu
@@ -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];
+}

Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

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

At a glance this does seem like the right place to do this, but this is still missing a release note.

It seems like GCC allows e.g. __attribute__(()) alignas(16) int x in any case, so I don’t see why we shouldn’t allow this too. Can you also add some tests that use __attribute__(()) directly and which aren’t CUDA-specific?

Oh, and can you add solmething like this as a test as well:

struct S { __attribute__((deprecated)) alignas(16) int x; };

@Sirraide
Copy link
Member

CC @erichkeane in case there’s a specific reason I’m not aware of as to why we currently don’t allow this.

@DenisGZM
Copy link
Contributor Author

DenisGZM commented Mar 28, 2025

At a glance this does seem like the right place to do this, but this is still missing a release note.

It seems like GCC allows e.g. __attribute__(()) alignas(16) int x in any case, so I don’t see why we shouldn’t allow this too. Can you also add some tests that use __attribute__(()) directly and which aren’t CUDA-specific?

Oh, and can you add solmething like this as a test as well:

struct S { __attribute__((deprecated)) alignas(16) int x; };

Actually this test doesn't work with this patch...

In this case all attributes are processed in ParseDeclarationSpecifiers, which in my first view was the right place to fix, but has way more complicated logic and easy to break diagnostics.

In ParseDeclarationSpecifiers we parse kw__attributes and other CXX11 Attributes and set bool AttrsLastTime = true to check that last parsed piece was attr. Later this block prohibit attributes with AttrsLastTime = false,

ParseDecl.cpp

    DoneWithDeclSpec:
      if (!AttrsLastTime)
        ProhibitAttributes(attrs);

And AttrsLastTime is always false in declarations of the form: <attributes> <type> <identifier> , because last token we parse is type

Another approach i tried is to add processing alignas-cxx11 just like it is done for C: kw__Alignas and kw_alignas (c23).
Well, it do the parsing but later it skips CXX11 attributes when correcting declaration type (assumed that attributes must have been processed before)

@Sirraide
Copy link
Member

Hmm, @erichkeane probably knows where this needs to be parsed then; I might take another look at this myself later (because I’m not sure either off the top of my head), but I’m rather busy today unfortunately...

@DenisGZM
Copy link
Contributor Author

I added parsing all attributes in ParseCXXClassMemberDeclaration before calling ParseDeclarationSpecifiers and it seems to solve problem, but it also changes annotation ranges for struct and class members

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants