This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add Parse and Sema support for RegularKeyword attributes
ClosedPublic

Authored by rsandifo-arm on Apr 19 2023, 3:23 AM.

Details

Summary

This patch adds the Parse and Sema support for RegularKeyword attributes,
following on from a previous patch that added Attr.td support.

The patch is quite large. However, nothing outside the tests is
specific to the first RegularKeyword attribute (__arm_streaming).
The patch should therefore be a one-off, up-front cost. Other
attributes just need an entry in Attr.td and the usual Sema support.

The approach taken in the patch is that the keywords can be used with
any language version. If standard attributes were added in language
version Y, the keyword rules for version X<Y are the same as they were
for version Y (to the extent possible). Any extensions beyond Y are
handled in the same way for both keywords and attributes. This ensures
that existing C++11 successors like C++17 are not treated differently
from versions that have yet to be defined.

Some notes on the implementation:

  • The patch emits errors rather than warnings for diagnostics that

relate to keywords.

  • Where possible, the patch drops “attribute” from diagnostics

relating to keywords.

  • One exception to the previous point is that warnings about C++

extensions do still mention attributes. The use there seemed OK
since the diagnostics are noting a change in the production rules.

  • If a diagnostic string needs to be different for keywords and

attributes, the patch standardizes on passing the attribute/
name/token followed by 0 for attributes and 1 for keywords.

  • Although the patch updates warn_attribute_wrong_decl_type_str,

warn_attribute_wrong_decl_type, and warn_attribute_wrong_decl_type,
only the error forms of these strings are used for keywords.

  • I couldn't trigger the warnings in checkUnusedDeclAttributes,

even for existing attributes. An assert on the warnings caused
no failures in the testsuite. I think in practice all standard
attributes would be diagnosed before this.

  • The patch drops a call to standardAttributesAllowed in

ParseFunctionDeclarator. This is because MaybeParseCXX11Attributes
checks the same thing itself, where appropriate.

  • The new tests are based on c2x-attributes.c and

cxx0x-attributes.cpp. The C++ test also incorporates a version of
cxx11-base-spec-attributes.cpp. The FIXMEs are carried across from
the originals.

Diff Detail

Event Timeline

rsandifo-arm created this revision.Apr 19 2023, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 3:23 AM
rsandifo-arm requested review of this revision.Apr 19 2023, 3:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2023, 3:23 AM

So I don't mind the changes in this stack, though this doing a little bit of a 'backdoor' way of getting the arm-streaming attribute in rubs me the wrong way. I'm not a huge fan that the solution we've got here only solves THIS problem, and doesn't extend to improving the situation with older attributes as well.

I would like someone more knowledgeable in parsing to take a look as well.

clang/lib/Sema/SemaDecl.cpp
5318–5320

Is this function (isRegularKeywordAttribute) checking spellings? If not, it needs to.

rsandifo-arm marked an inline comment as done.Apr 19 2023, 7:24 AM

Thanks for the reviews!

So I don't mind the changes in this stack, though this doing a little bit of a 'backdoor' way of getting the arm-streaming attribute in rubs me the wrong way. I'm not a huge fan that the solution we've got here only solves THIS problem, and doesn't extend to improving the situation with older attributes as well.

Yeah, it looks a bit over-the-top/special-purpose when __arm_streaming is the only thing using it. But we (Arm) have other “semantic attributes” in the pipeline, and this would be useful for them too. Hopefully other targets will find the same.

We could even go back and retroactively support keywords for some existing Arm semantic attributes. E.g. maybe we could allow __aarch64_vector_pcs alongside __attribute__((aarch64_vector_pcs)). I'd have to see what others think.

But as far as I could tell, there are no existing keyword attributes whose grammar is close enough to standard attributes for the keywords to use the new infrastructure. E.g. many existing keywords are allowed between qualifiers, whereas standard attributes aren't:

int (__stdcall a1)(); // OK, but standard attributes aren't allowed in this position
extern int (*const __stdcall volatile a2) (); // OK, but standard attributes wouldn't be allowed here

I don't think we can retroactively forbid these. But I don't think it makes sense to allow new attributes like __arm_streaming in these positions either.

clang/lib/Sema/SemaDecl.cpp
5318–5320

Yeah, this is checking the spelling. In principle, the series supports attributes that have a RegularKeyword spelling and some other spelling, and in that case, this check would only include attributes that were written using the RegularKeyword spelling.

rsandifo-arm marked an inline comment as done.

Update onto current main. Only change is to replace C++2b with C++23.

So I have 1 issue that is throughout the patch, but I'm ok with the patch otherwise. This'll get a LGTM after that is fixed.

clang/lib/Sema/SemaDeclAttr.cpp
2904

Every where you are doing just a '0' in a diagnostic here it makes it incredibly unreadable. I'd prefer 1 of 2 solutions:

1- Create an enum somewhere that encodes the meaning here, and use those instead.
2- use a /*Whatever*/ comment every place you're passing a raw literal.

Avoid most uses of << 0. Add comments to those that remain.

erichkeane accepted this revision.May 30 2023, 7:24 AM

1 nit, feel free to do this as a part of the commit.

clang/lib/Sema/ParsedAttr.cpp
208

Humorously, I spent an absurd amount of time trying to figure out whether appurtenance was a word/the correct spelling, determined you had it right and closed the phab review box, just to see atributes :D

This revision is now accepted and ready to land.May 30 2023, 7:24 AM
rsandifo-arm marked an inline comment as done.May 30 2023, 7:26 AM
rsandifo-arm added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
2904

Yeah, that's fair. Sorry about that. I got some of my own medicine reading the patch back after a while away from it.

Most of those << 0 come from looking at individual uses to see whether a regular keyword spelling is ever possible. If it wasn't possible, << 0 was supposed to be a justification for not covering that line of code in the new test cases.

But I now think that was a mistake. Using isRegularKeywordAttribute() is useful for readability even if we “know” what its value ahead of time. And using it is more future-proof as well.

So this update converts most uses of << 0 to << Something.isRegulardKeywordAttribute in cases where the appropriate Something is readily available. There are a handful of cases where an immediate is still needed, so I went for option (2) and added a comment to those.

rsandifo-arm marked an inline comment as done.

Rebase to account for a diagnostic that has moved.
No other changes from the previous version.

I'll wait for the build results before pushing.

Thanks for the review!