[clang] Improve `_Alignas` on a `struct` declaration diagnostic #65638

jerinphilip · 2023-09-07T16:51:32Z

Adds isAlignas() method on AttributeCommonInfo which accounts for
C++ alignas as well as C11 _Alignas.

The method is used to improve diagnostic in C when _Alignas is used in
C at the wrong location. This corrects the previously suggested move
of _Alignas past the declaration specifier, now warns attribute
_Alignas is ignored.

Based on https://reviews.llvm.org/D141177.

cor3ntin · 2023-09-08T07:52:45Z

Generally looks good, thanks for working on this

I'd like to see more tests in clang/test/Sema/alignas.c - we don't have a test for struct _Alignas(int) T either apparently.
Given this fixes a bug, it could benefit from a release note in clang/docs/ReleaseNotes.rst

cor3ntin · 2023-09-08T10:30:57Z

There is similar effort on Phab, we should pick one of them https://reviews.llvm.org/D141177

cor3ntin · 2023-10-03T16:38:15Z

@AaronBallman you have a preference as to which PR we pursue?

AaronBallman · 2023-10-05T18:58:12Z

@AaronBallman you have a preference as to which PR we pursue?

No strong preference, but the author of D141177 seems to have gone silent again so if @jerinphilip is actively working on this patch, I'd say let's go with this one.

cor3ntin

LGTM modulo nitpick.
Maybe @AaronBallman will want to look at it too.
Do we want a release notes for this?

clang/lib/Sema/SemaDecl.cpp

AaronBallman

Thank you for working on this! I think the changes should come with a release note so users know about the fix. I did have a question on the design, but overall the direction seems sensible to me.

clang/include/clang/Basic/AttributeCommonInfo.h

clang/test/Parser/c2x-alignas.c

clang/include/clang/Basic/AttributeCommonInfo.h

jerinphilip · 2023-10-22T18:44:10Z

I have rebased the PR with main (which appears to fix the formatting workflow). For the time being, I have left the C23 test using alignas in with the active error message and a FIXME.

I've altered the state of this PR to be close to D141177, using the existing attribute warning for reduced complexity (happy to provide attribution). Key difference isAlignas() instead of isC23AlignasAttribute(), thinking the former should be more future-proof. In the current state, isCXX11Attribute() and isStandardAttributeSyntax() is not modified, and the net improvement (in diagnostics) in changes here should be useful when C23 alignas comes in, I hope (I would like to pursue this as a follow-up separate undertaking, if okay).

The following routes were also considered:

Store tok::TokenKind to resolve _Alignas vs alignas. This requires adding one more field to Form and AttributeCommonInfo, while the information is already present via (IsAlignas, SpellingIndex) tuple (This approach is in jerinphilip/llvm-project#1 mixed with other things).
Use calculateAttributeSpellingIndex (also discussed in meeting). This suffers from inability to use AlignedAttr::Keyword_Alignas (clang/AST/Attrs.h) - See #65638 (comment). Absent this, the SpellingIndex value 5 (Keyword_Alignas) will have to be maintained separately here to resolve _Alignas. Looking at the source requires string operations, which are inefficient.

AaronBallman

Aside from some nits, I think this looks reasonable to me. I'll leave the final sign-off to @erichkeane but if he doesn't approve in the next few days (WG21 meetings are upcoming so he may be busy), I'll come back to approve.

Thank you for all the help on this one! It turns out good first issue is really hard to determine accurately. ;-)

AaronBallman · 2023-11-01T13:26:07Z

clang/docs/ReleaseNotes.rst

+  declaration specifiers.
+


Suggested change

declaration specifiers.

declaration specifiers.

(#58637 <https://github.com/llvm/llvm-project/issues/58637>`_)

AaronBallman · 2023-11-01T13:29:31Z

clang/lib/Sema/SemaDecl.cpp

+        unsigned DiagnosticId;
+        if (AL.isAlignas() && !getLangOpts().CPlusPlus) {
+          DiagnosticId = diag::warn_attribute_ignored;
+        } else if (AL.isRegularKeywordAttribute()) {
+          DiagnosticId = diag::err_declspec_keyword_has_no_effect;
+        } else {
+          DiagnosticId = diag::warn_declspec_attribute_ignored;
+        }


Suggested change

unsigned DiagnosticId;

if (AL.isAlignas() && !getLangOpts().CPlusPlus) {

DiagnosticId = diag::warn_attribute_ignored;

} else if (AL.isRegularKeywordAttribute()) {

DiagnosticId = diag::err_declspec_keyword_has_no_effect;

} else {

DiagnosticId = diag::warn_declspec_attribute_ignored;

}

unsigned DiagnosticId = diag::warn_declspec_attribute_ignored;

if (AL.isAlignas() && !getLangOpts().CPlusPlus)

DiagnosticId = diag::warn_attribute_ignored;

else if (AL.isRegularKeywordAttribute())

DiagnosticId = diag::err_declspec_keyword_has_no_effect;

Changes for our odd choice coding style and to be slightly more succinct.

clang/test/Parser/cxx0x-attributes.cpp

erichkeane

The comments Aaron made should be fixed, plus 1 more from me, else LGTM.

erichkeane · 2023-11-01T14:05:01Z

clang/include/clang/Basic/AttributeCommonInfo.h

@@ -192,6 +192,13 @@ class AttributeCommonInfo {

  bool isC23Attribute() const { return SyntaxUsed == AS_C23; }

+  bool isAlignas() const {
+    // In the current state of code, IsAlignas is only configured to return


This comment is immediately stale once we commit this, right? Also, it refers to IsAlignas, but the function name is isAlignas.

This refers to the variable IsAlignAs (not the parent function isAlignas), and the sentence holds for the current state of code. I believe I'm trying to explain why I cannot use IsAlignas's current behaviour here (as it is more invasive). Normally I'd expect this function to be a const accessor to a private member per programming conventions, which is not the case here, hence a comment.

I'm sensing the review comment confusion indicates I need to communicate this better in source comments. Suggestions welcome.

How about:

// FIXME: In the current state, the IsAlignas member variable is only true with the C++ // `alignas` keyword but not `_Alignas`. The following expression works around the // otherwise lost information so it will return true for `alignas` or `_Alignas` while still // returning false for things like `__attribute__((aligned))`.

Adds `isAlignas()` method on `AttributeCommonInfo` which accounts for C++ `alignas` as well as C11 `_Alignas`. The method is used to improve diagnostic in C when `_Alignas` is used in C at the wrong location. This corrects the previously suggested move of `_Alignas` past the declaration specifier, now warns attribute `_Alignas` is ignored.

AaronBallman

LGTM! Thank you for the fix!

jerinphilip requested a review from a team as a code owner September 7, 2023 16:51

github-actions bot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Sep 7, 2023

cor3ntin reviewed Oct 10, 2023

View reviewed changes

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

AaronBallman reviewed Oct 11, 2023

View reviewed changes

clang/include/clang/Basic/AttributeCommonInfo.h Outdated Show resolved Hide resolved

AaronBallman requested a review from erichkeane October 11, 2023 13:53

erichkeane reviewed Oct 11, 2023

View reviewed changes

clang/include/clang/Basic/AttributeCommonInfo.h Outdated Show resolved Hide resolved

clang/include/clang/Basic/AttributeCommonInfo.h Outdated Show resolved Hide resolved

llvmbot added the clang Clang issues not falling into any other category label Oct 16, 2023

jerinphilip commented Oct 17, 2023

View reviewed changes

clang/test/Parser/c2x-alignas.c Outdated Show resolved Hide resolved

jerinphilip commented Oct 20, 2023

View reviewed changes

clang/include/clang/Basic/AttributeCommonInfo.h Outdated Show resolved Hide resolved

jerinphilip force-pushed the alignas_capture__Alignas branch 3 times, most recently from 74b62ef to 919be6b Compare October 22, 2023 18:40

AaronBallman reviewed Nov 1, 2023

View reviewed changes

erichkeane reviewed Nov 1, 2023

View reviewed changes

jerinphilip force-pushed the alignas_capture__Alignas branch from 919be6b to 323e569 Compare November 1, 2023 20:43

jerinphilip force-pushed the alignas_capture__Alignas branch from 323e569 to 947d4b4 Compare November 2, 2023 01:51

AaronBallman approved these changes Nov 3, 2023

View reviewed changes

AaronBallman merged commit 2e79133 into llvm:main Nov 7, 2023

This was referenced Nov 8, 2023

fix empty bb maksfb/llvm-project#2

Closed

fix empty bb maksfb/llvm-project#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Improve `_Alignas` on a `struct` declaration diagnostic #65638

[clang] Improve `_Alignas` on a `struct` declaration diagnostic #65638

jerinphilip commented Sep 7, 2023 •

edited

Loading

cor3ntin commented Sep 8, 2023

cor3ntin commented Sep 8, 2023

cor3ntin commented Oct 3, 2023

AaronBallman commented Oct 5, 2023

cor3ntin left a comment

AaronBallman left a comment

jerinphilip commented Oct 22, 2023 •

edited

Loading

AaronBallman left a comment

AaronBallman Nov 1, 2023

AaronBallman Nov 1, 2023

erichkeane left a comment

erichkeane Nov 1, 2023

jerinphilip Nov 1, 2023

AaronBallman Nov 1, 2023

AaronBallman left a comment

	declaration specifiers.
	declaration specifiers.
	(#58637 <https://github.com/llvm/llvm-project/issues/58637>`_)

[clang] Improve _Alignas on a struct declaration diagnostic #65638

[clang] Improve _Alignas on a struct declaration diagnostic #65638

Conversation

jerinphilip commented Sep 7, 2023 • edited Loading

cor3ntin commented Sep 8, 2023

cor3ntin commented Sep 8, 2023

cor3ntin commented Oct 3, 2023

AaronBallman commented Oct 5, 2023

cor3ntin left a comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

jerinphilip commented Oct 22, 2023 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Nov 1, 2023

Choose a reason for hiding this comment

AaronBallman Nov 1, 2023

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Nov 1, 2023

Choose a reason for hiding this comment

jerinphilip Nov 1, 2023

Choose a reason for hiding this comment

AaronBallman Nov 1, 2023

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

[clang] Improve `_Alignas` on a `struct` declaration diagnostic #65638

[clang] Improve `_Alignas` on a `struct` declaration diagnostic #65638

jerinphilip commented Sep 7, 2023 •

edited

Loading

jerinphilip commented Oct 22, 2023 •

edited

Loading