Fix a regression with alignas on structure members in C

[AaronBallman]

This was a 19.x regression and thus has no release note.

Fixes #95032

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

This was a 19.x regression and thus has no release note.

Fixes #95032

---

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

5 Files Affected:

• (modified) clang/include/clang/Sema/DeclSpec.h (+2-1)
• (modified) clang/lib/Parse/ParseDecl.cpp (+10)
• (modified) clang/lib/Sema/ParsedAttr.cpp (+1-1)
• (modified) clang/test/C/C2y/n3254.c (+2-2)
• (modified) clang/test/Sema/alignas.c (+16-3)

diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 9c22c35535ede..1dd9781bba5ea 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -2042,7 +2042,8 @@ class Declarator {
     assert(llvm::all_of(DeclarationAttrs,
                         [](const ParsedAttr &AL) {
                           return (AL.isStandardAttributeSyntax() ||
-                                  AL.isRegularKeywordAttribute());
+                                  AL.isRegularKeywordAttribute() ||
+                                  AL.isAlignas());
                         }) &&
            "DeclarationAttrs may only contain [[]] and keyword attributes");
   }
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7ce9a9cea1c7a..ae1801f8a572a 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -4919,6 +4919,16 @@ void Parser::ParseStructDeclaration(
   // Parse the common specifier-qualifiers-list piece.
   ParseSpecifierQualifierList(DS);
 
+  // FIXME: _Alignas has special handling that really shouldn't be necessary.
+  // Loop over all the attributes on the specifier qualifier, find any that
+  // are alignment attributes, and shift those off the specifier qualifier and
+  // onto the declarator.
+  ParsedAttributes &DSAttrs = DS.getAttributes();
+  for (auto &PAttr : DSAttrs) {
+    if (PAttr.isAlignas())
+      Attrs.takeOneFrom(DSAttrs, &PAttr);
+  }
+
   // If there are no declarators, this is a free-standing declaration
   // specifier. Let the actions module cope with it.
   if (Tok.is(tok::semi)) {
diff --git a/clang/lib/Sema/ParsedAttr.cpp b/clang/lib/Sema/ParsedAttr.cpp
index 6abc90336c994..2109494aa5889 100644
--- a/clang/lib/Sema/ParsedAttr.cpp
+++ b/clang/lib/Sema/ParsedAttr.cpp
@@ -225,7 +225,7 @@ bool ParsedAttr::slidesFromDeclToDeclSpecLegacyBehavior() const {
     // atributes.
     return false;
 
-  assert(isStandardAttributeSyntax());
+  assert(isStandardAttributeSyntax() || isAlignas());
 
   // We have historically allowed some type attributes with standard attribute
   // syntax to slide to the decl-specifier-seq, so we have to keep supporting
diff --git a/clang/test/C/C2y/n3254.c b/clang/test/C/C2y/n3254.c
index e08659cf377aa..347360a87faed 100644
--- a/clang/test/C/C2y/n3254.c
+++ b/clang/test/C/C2y/n3254.c
@@ -80,9 +80,9 @@ struct T {
 // CHECK-LABEL: define dso_local signext i8 @quux(
 // CHECK-SAME: ) #[[ATTR0]] {
 // CHECK-NEXT:  [[ENTRY:.*:]]
-// CHECK-NEXT:    [[T:%.*]] = alloca [[STRUCT_T:%.*]], align 1
+// CHECK-NEXT:    [[T:%.*]] = alloca [[STRUCT_T:%.*]], align 4
 // CHECK-NEXT:    [[S_PTR:%.*]] = alloca ptr, align 8
-// CHECK-NEXT:    call void @llvm.memset.p0.i64(ptr align 1 [[T]], i8 0, i64 12, i1 false)
+// CHECK-NEXT:    call void @llvm.memset.p0.i64(ptr align 4 [[T]], i8 0, i64 12, i1 false)
 // CHECK-NEXT:    [[BUFFER:%.*]] = getelementptr inbounds [[STRUCT_T]], ptr [[T]], i32 0, i32 0
 // CHECK-NEXT:    [[ARRAYDECAY:%.*]] = getelementptr inbounds [12 x i8], ptr [[BUFFER]], i64 0, i64 0
 // CHECK-NEXT:    store ptr [[ARRAYDECAY]], ptr [[S_PTR]], align 8
diff --git a/clang/test/Sema/alignas.c b/clang/test/Sema/alignas.c
index 020eff6a141c0..391553bc540ec 100644
--- a/clang/test/Sema/alignas.c
+++ b/clang/test/Sema/alignas.c
@@ -1,5 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -Dalignof=__alignof %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -Dalignof=_Alignof -DUSING_C11_SYNTAX %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c23 -DUSING_C11_SYNTAX %s
 
 _Alignas(3) int align_illegal; //expected-error {{requested alignment is not a power of 2}}
 _Alignas(int) char align_big;
@@ -18,12 +19,24 @@ void f(_Alignas(1) char c) { // expected-error {{'_Alignas' attribute cannot be
 }
 
 #ifdef USING_C11_SYNTAX
-// expected-warning@+4{{'_Alignof' applied to an expression is a GNU extension}}
-// expected-warning@+4{{'_Alignof' applied to an expression is a GNU extension}}
-// expected-warning@+4{{'_Alignof' applied to an expression is a GNU extension}}
+// expected-warning-re@+4{{'{{(_A|a)}}lignof' applied to an expression is a GNU extension}}
+// expected-warning-re@+4{{'{{(_A|a)}}lignof' applied to an expression is a GNU extension}}
+// expected-warning-re@+4{{'{{(_A|a)}}lignof' applied to an expression is a GNU extension}}
 #endif
 _Static_assert(alignof(align_big) == alignof(int), "k's alignment is wrong");
 _Static_assert(alignof(align_small) == 1, "j's alignment is wrong");
 _Static_assert(alignof(align_multiple) == 8, "l's alignment is wrong");
 _Static_assert(alignof(struct align_member) == 8, "quuux's alignment is wrong");
 _Static_assert(sizeof(struct align_member) == 8, "quuux's size is wrong");
+
+struct GH95032_1 {
+  _Alignas(16) char bytes[16];
+};
+_Static_assert(_Alignof(struct GH95032_1) == 16, "");
+
+#if __STDC_VERSION__ >= 202311L
+struct GH95032_2 {
+  alignas(16) char bytes[16];
+};
+static_assert(alignof(struct GH95032_2) == 16);
+#endif

[jyknight]

I'm not sure I understand what happened here, since _Alignas did work properly before this change, right? How/why are _Alignas and alignas acting differently from eachother in C23 mode?

[AaronBallman]

I'm not sure I understand what happened here, since _Alignas did work properly before this change, right? How/why are _Alignas and alignas acting differently from eachother in C23 mode?

_Alignas on a field used to work properly before b9cf7f1, but that commit changed alignas handling improperly. The issue boils down to the fact that C++ treats alignas as an attribute and C treats _Alignas (and now alignas as of C23) as a type specifier rather than an attribute. We were originally leaning on the C++ implementation when the keyword was spelled alignas and we missed some changes to make that work in C, which is what this commit is addressing (by ensuring the attribute moves onto the declarator where it belongs and used to be found).

[jyknight]

The part I'm confused about is that this commit doesn't appear to simply be adding "or alignas" to some code that said "if _Alignas". Yes, the newly added isAlignas() checks do cover both attributes, but _Alignas was still working even after b9cf7f1.

That is, in clang trunk, this test does still pass when parsed as -std=c23:

struct X {
    _Alignas(8) char n;
};

static_assert(_Alignof(struct X) == 8, "");

So why does this PR need to add new code to move alignas, instead of having alignas use whatever codepath was already handling _Alignas? Or, alternatively remove whatever code was formerly handling _Alignas, in favor of this new code?

Or, are alignas and _Alignas actually getting parsed differently, even still, so only alignas needs to be moved?

[AaronBallman]

The part I'm confused about is that this commit doesn't appear to simply be adding "or alignas" to some code that said "if _Alignas". Yes, the newly added isAlignas() checks do cover both attributes, but _Alignas was still working even after b9cf7f1.

That is, in clang trunk, this test does still pass when parsed as -std=c23:

struct X {
    _Alignas(8) char n;
};

static_assert(_Alignof(struct X) == 8, "");

So why does this PR need to add new code to move alignas, instead of having alignas use whatever codepath was already handling _Alignas? Or, alternatively remove whatever code was formerly handling _Alignas, in favor of this new code?

Or, are alignas and _Alignas actually getting parsed differently, even still, so only alignas needs to be moved?

Ah, thank you for the further details! The issue boils down to the fact that alignas is not an attribute in C23 (it's a type-specifier-qualifier) but it is an attribute in C++. So we hit awkward situations where we claim the attribute is a C++11-style attribute, such as in ProcessDeclAttribute(). We explicitly disallow C++11 attributes on the declaration specifier:

  // Apply decl attributes from the DeclSpec if present.
  if (!PD.getDeclSpec().getAttributes().empty()) {
    ProcessDeclAttributeList(S, D, PD.getDeclSpec().getAttributes(),
                             ProcessDeclAttributeOptions()
                                 .WithIncludeCXX11Attributes(false)
                                 .WithIgnoreTypeAttributes(true));
  }

but when the attribute is on the declaration itself, we allow C++11 attributes:

ProcessDeclAttributeList(S, D, NonSlidingAttrs);

So there is a better way to address this and I think it actually fixes a bug that I didn't notice until now with non-structure use. The changes look a bit odd because I also had to make an adjustment to ensure that matrix_type continues to work (it's an odd beast that already required special handling, that special handling is now needed in two places).

[jyknight]

Thanks, LGTM. I like this version.

Did this change also fix any other bugs? Since it looks like we were previously not skipping ProcessDeclAttribute for [[]] attributes in C23 mode, when IncludeCXX11Attributes == false, and we were supposed to have been?

[AaronBallman]

Thanks, LGTM. I like this version.

Thank you for the excellent review, I think we came to a better approach in the end!

Did this change also fix any other bugs? Since it looks like we were previously not skipping ProcessDeclAttribute for [[]] attributes in C23 mode, when IncludeCXX11Attributes == false, and we were supposed to have been?

Theoretically yes, but practically no. We don't have any attributes in C23 that I think would be impacted by this except for alignas. At least, I've not been able to induce a difference in behavior.