[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations
Martin Böhme via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 31 13:04:55 PDT 2022
mboehme marked 8 inline comments as done.
mboehme added a comment.
I notice that https://reviews.llvm.org/D111548, which this change is based on, has some failing flang tests in the CI. I'm pretty sure these are unrelated and stem from the base revision that I happen to be based off. Before rebasing, however, I wanted to make sure all of the big comments have been resolved so that I don't introduce too much churn into the diffs.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
/// Attrs - Attributes.
ParsedAttributes Attrs;
+ ParsedAttributes DeclarationAttrs;
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > We should probably rename `Attrs` to be less generic and add some comments to explain why there are two lists of parsed attributes.
> > I've added some comments to clarify the difference. I'm not sure what a good alternative name for `Attrs` would be though. The only obvious candidate that comes to mind is `DeclaratorAttrs`, but that seems pretty redundant given that this is a field of `Declarator`.
> >
> > I think there is actually a good case for calling the first list simply `Attrs` because, unlike `DeclarationAttrs`, it applies directly to the `Declarator`. This distinction is amplified by the fact that, since your review, I've changed `DeclarationAttrs` to be a reference, not a by-value field.
> The new comment actually makes it less clear for me -- that says the attributes are for the `DeclSpec` but they're actually for the `Declatator` (there's no relationship between `Attrs` and `DS`).
>
> Given that `DeclSpec` uses `Attrs` for its associated attributes, I think it's okay to use `Attrs` here for the ones associated with the `Declarator`, but the comment should be updated in that case.
Sorry, I don't know what I was thinking when I wrote `DeclSpec`. That should have read "Attributes attached to the declarator", and I've changed it accordingly.
I hope that with this change, this makes sense now?
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+ assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) {
+ return AL.isStandardAttributeSyntax();
+ }));
----------------
rsmith wrote:
> rsmith wrote:
> > I think I recall seeing OpenMP pragma attributes being parsed as declaration attributes too. FIXME update this comment after review is complete
> Oops, forgot to circle back to this. I think I was wrong; please disregard :)
Just making sure I can disregard this comment? Please respond if there's something you'd like me to address here.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+ /// This may only be called if isStandardAttributeSyntax() returns true.
+ bool slidesFromDeclToDeclSpec() const;
+
----------------
aaron.ballman wrote:
> Because this is now specific to standard attributes, and those don't "slide" under normal circumstances, it might be nice to rename this method to make it more clear that this should not be called normally. I don't have a strong opinion on what a *good* name is, but even something like `improperlySlidesFromDeclToDeclSpec()` would make me happy.
I've decided to add `LegacyBehavior` at the end -- WDYT?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ ProhibitAttributes(DeclAttrs);
+ ProhibitAttributes(OriginalDeclSpecAttrs);
DeclEnd = Tok.getLocation();
----------------
aaron.ballman wrote:
> rsmith wrote:
> > This looks wrong to me (both before and after this patch; you've faithfully retained an incorrect behavior). If `DeclSpec` attributes aren't allowed, they should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after this patch, we'll diagnose attributes in a free-standing decl-specifier-seq if we happened to tentatively parse them, not if we happened to parse them in `ParseDeclarationSpecifiers`. For example:
> >
> > ```
> > __attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
> > void f() {
> > __attribute(()) struct X; // DeclSpecAttrs will contain the attribute specifier, error
> > }
> > ```
> >
> > Comparing to GCC's behavior for that example (which GCC silently accepts) and for this one:
> >
> > ```
> > __attribute__((packed)) struct X; // GCC warns that packed has no meaning here, clang produces a high-quality warning.
> > void f() {
> > __attribute((packed)) struct X; // GCC warns that packed has no meaning here, clang produces a bogus error.
> > }
> > ```
> >
> > ... I think the right thing to do is to delete this call (along with `OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently syntactically permitted here, but most (maybe even all?) GNU attributes don't have any meaning in this position.
> Good catch!
Thanks for pointing this out!
I've removed the `ParseAttributes(DeclSpecAttrs)` call and have added tests for the behavior of free-standing decl-specifier-seqs in test/Parser/attributes.c (for empty `__attribute__(())`) and test/Sema/attr-declspec-ignored.c (to make sure that we produce the correct warnings on block declarations).
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2691-2695
+ // We need to keep these attributes for future diagnostics
+ // before they are taken over by the declaration.
+ ParsedAttributesView FnAttrs;
+ FnAttrs.addAll(DeclAttrs.begin(), DeclAttrs.end());
+ FnAttrs.Range = DeclAttrs.Range;
----------------
rsmith wrote:
> Oh, neat, I think this copy isn't necessary any more, given that we don't give ownership of the `DeclAttrs` to the `ParsingDeclarator` any more, and don't mix the declaration attributes and the `DeclSpec` attributes together any more. We should be able to just use `DeclAttrs` instead of `FnAttrs` below now, I think?
Yes, done. Nice!
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:2745-2746
// ptr-operators.
- Declarator D(DS, DeclaratorContext::ConversionId);
+ ParsedAttributes EmptyDeclAttrs(AttrFactory);
+ Declarator D(DS, EmptyDeclAttrs, DeclaratorContext::ConversionId);
ParseDeclaratorInternal(D, /*DirectDeclParser=*/nullptr);
----------------
aaron.ballman wrote:
> rsmith wrote:
> > Given the number of times this has come up, I wonder if it's worth adding something like:
> >
> > ```
> > class ParsedAttributes {
> > public:
> > // ...
> > static const ParsedAttributes &none() {
> > static AttributeFactory Factory;
> > static AttributePool Pool(Factory);
> > static const ParsedAttributes Attrs(Pool);
> > return Attrs;
> > }
> > // ...
> > };
> > ```
> > (or, better, changing `Declarator` to hold a `const ParsedAttributesView&` rather than a `const ParsedAttributes&`) so that we can write
> > ```
> > Declarator D(DS, ParsedAttributes::none(), DeclaratorContext::ConversionId);
> > ```
> >
> > Totally optional, though, and I'm happy for this cleanup to happen at some later point in time.
> +1 to this being a pretty nice cleanup and it being fine if it happens in a follow-up.
Good point -- done (and changed `Declarator` to hold a `const ParsedAttributesView&`).
================
Comment at: clang/lib/Parse/ParseStmt.cpp:16
#include "clang/Basic/Attributes.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/PrettyStackTrace.h"
----------------
rsmith wrote:
> This header is intended to be private to Sema. If you want to use a diagnostic in both the parser and sema, please move it to `DiagnosticCommonKinds.td`.
Thanks, done.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:242-246
+ Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+ GNUAttrs, &GNUAttributeLoc);
} else {
- Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, Attrs);
+ Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+ GNUAttrs);
----------------
rsmith wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > I think this is the only place where we're passing decl-specifier-seq attributes into `ParseDeclaration`. There are only two possible cases here:
> > > >
> > > > 1) We have a simple-declaration, and can `ParseSimpleDeclaration` directly.
> > > > 2) We have a static-assert, using, or namespace alias declaration, which don't permit attributes at all.
> > > >
> > > > So I think we *could* simplify this so that decl-spec attributes are never passed into `ParseDeclaration`:
> > > >
> > > > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> > > > * Otherwise, call `ParseSimpleDeclaration` and pass in the attributes.
> > > > * Remove the `DeclSpecAttrs` list from `ParseDeclaration`.
> > > >
> > > > I'm not requesting a change here -- I'm not sure whether that's a net improvement or not -- but it seems worth considering.
> > > I think this is a good avenue to explore -- passing in two different attribute lists means someone will at some point get it wrong by accident, so only having one attribute list reduces the chances for bugs later. I don't imagine static assertions or namespaces will get leading attributes. However...
> > >
> > > I think asm-declaration and using-directive are also a bit special -- they're allowed to have leading attributes: http://eel.is/c++draft/dcl.dcl#nt:asm-declaration and http://eel.is/c++draft/dcl.dcl#nt:using-directive
> > >
> > > Do we also have to handle opaque-enum-declaration here? http://eel.is/c++draft/dcl.dcl#nt:block-declaration
> > I investigated this, but I'm not sure it's a net win.
> >
> > > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> >
> > Or if the next token is `kw_inline` and the token after that is `kw_namespace`...
> >
> > I think going down this path would mean duplicating all of the case distinctions in `ParseDeclaration()` -- and if we add more cases in the future, we'd have to make sure to replicate them here. I think overall it keeps the code simpler to accept the additional `DeclSpecAttrs` parameter.
> >
> > > Do we also have to handle opaque-enum-declaration here? http://eel.is/c++draft/dcl.dcl#nt:block-declaration
> >
> > A moot point, probably, but I believe this is handled by `ParseEnumSpecifier()`, which is called from `ParseDeclarationSpecifiers()`, so there would be no need to handle it separately.
> > > * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> >
> > Or if the next token is `kw_inline` and the token after that is `kw_namespace`...
>
> This function is parsing block declarations, so we don't have to handle `inline namespace` here because that can't appear as a block declaration. The only reason we need to handle `namespace` is to support namespace aliases (`namespace A = B;`), which are (perhaps surprisingly) allowed at block scope.
>
> > I think going down this path would mean duplicating all of the case distinctions in `ParseDeclaration()` -- and if we add more cases in the future, we'd have to make sure to replicate them here. I think overall it keeps the code simpler to accept the additional `DeclSpecAttrs` parameter.
>
> It's not all the cases, only the block-declaration cases. If we wanted to directly follow the grammar, we could split a `ParseBlockDeclaration` function out of `ParseDeclaration`, but I guess there'd be so little left in `ParseDeclaration` that it wouldn't be worth it, and we'd still have to pass the `DeclSpecAttrs` parameter to `ParseBlockDeclaration`, so that doesn't save us anything. I suppose it'd also regress our diagnostic quality for declarations that aren't allowed at block scope. OK, I'm convinced :-)
Just confirming: You're not asking me to do anything here, correct?
================
Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
case tok::kw_asm: {
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
bool msAsm = false;
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > It seems like we might have a pre-existing bug here for [[]] attributes? http://eel.is/c++draft/dcl.dcl#nt:asm-declaration
> > Yes, we do!
> >
> > Fixed, and tests added in test/Parser/asm.c and test/Parser/asm.cpp.
> I think we should prohibit this for all `[[]]` attributes. C doesn't have an asm statement as part of the standard, but it lists it as a common extension in Annex J and uses the same syntax as the C++ feature. Might as well keep it consistent between the languages.
My rationale for handling C and C++ differently here was because they treat `asm` differently.
In C++, the standard requires us to allow attributes, so we merely warn that they will be ignored. (Also, `asm` is formally a declaration, not a statement in C++, though I’m not sure that makes a difference to the syntax.)
In C, J.5.10 in “Common extensions” says “The most common implementation is via a statement [...]”. C permits attributes only on declarations, though since `asm` is an extension, that’s not really relevant anyway. Since this is our extension and we wouldn’t do anything with the attributes anyway, it seemed best to me to prohibit them outright.
OTOH, I’ve just checked and GCC appears to accept attributes in this position even in C mode. Is the compatibility argument strong enough that we should do this too?
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243
+ case AT_Regparm:
+ case AT_NoDeref:
+ case AT_ObjCGC:
+ case AT_VectorSize:
----------------
rsmith wrote:
> Many of these were previously not permitted in type attribute position, despite being declared in Attr.td as `TypeAttr`s. Do they actually work in type attribute position after this patch? At least `matrix_type` currently expects to be applied only to a typedef declaration, so I'd expect that one does not work as a type attribute.
> Many of these were previously not permitted in type attribute position, despite being declared in Attr.td as `TypeAttr`s. Do they actually work in type attribute position after this patch?
Good point -- should have thought to test this myself.
I have now added tests for all of the "sliding attributes". It was good thing I did, because I realized that the logic in `processTypeAttrs()` was only letting very specific attributes through. The new logic is now much more generic (see SemaType.cpp:8278 and following).
I also discovered some interesting special cases that I needed to add extra code for.
> At least `matrix_type` currently expects to be applied only to a typedef declaration, so I'd expect that one does not work as a type attribute.
Thanks for pointing this out -- I had completely overlooked this. This is the first of the special cases I referred to above. It has unfortunately required adding some special-case code (SemaType.cpp:1819 and SemaDeclAttr.cpp:9311) to make sure we emit the right diagnostics. I've also added tests to make sure we emit the right diagnostics. The redeeming factor is that I think it will be possible to remove this code once we eliminate the "legacy sliding" behavior.
I have to say I find it unusual that a type attribute restricts itself to being used only on certain kinds of declarations; this seems a bit non-orthogonal. Is there a good reason for this, or could the attribute be "opened up" for use on types regardless of the context in which they're used?
I've confirmed by the way that none of the other "sliding" attributes is restricted to being used in a typedef, and I've added tests that demonstrate this if they didn't exist already.
The other two special cases are:
# `regparm`. This is a case where it's actually not correct to slide the attribute to the `DeclSpec` because it should instead be applied to the function type. Again, this attribute requires some special-case code; see SemaDeclAttr.cpp:8397 and SemaType.cpp:8467. The comments there explain the situation in more detail. Because the attribute doesn't actually "slide", I've removed it from the list of attributes in `slidesFromDeclToDeclSpecLegacyBehavior()`.
# `noderef`. It turns out that this attribute currently does not work at all in the `[[]]` spelling; I've written up a [bug](https://github.com/llvm/llvm-project/issues/55790) with the details. Because of this, there is no legacy behavior that needs to be preserved. Instead, for the time being, I have chosen to simply emit an "attribute ignored" warning when the attribute is encountered in `[[]]` spelling. Again, I have removed the attribute from the list of attributes in `slidesFromDeclToDeclSpecLegacyBehavior()`.
I have added tests that check the specific behavior of the `regparm` and `noderef` attributes.
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:224
+ // property is consciously not defined as a flag in Attr.td because we don't
+ // want new attributes to specify it.
+ switch (getParsedKind()) {
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > Thoughts on the additional comment? And should we start to issue that deprecation warning now (perhaps as a separate follow-up commit)?
> > > Thoughts on the additional comment?
> >
> > Yes, good point, we should point this out explicitly.
> >
> > I've worded the comment slightly differently: we can definitely deprecate the "sliding" behavior for attributes in the `clang::` namespace, as we own them, but there may be compatibility concerns for other attributes. This may mean that we can never reduce this list to nothing, but we should try.
> >
> > > And should we start to issue that deprecation warning now (perhaps as a separate follow-up commit)?
> >
> > We're already doing this (see the tests in test/SemaCXX/adress-space-placement.cpp), though again only for attributes in the `clang::` namespace, as we don't really have the authority to deprecate this usage for other attributes.
> >
> > I think the other question here is whether, by default, this should be an error, not a warning, but this would then presumably require a command-line flag to downgrade the error to a warning if needed (I believe @rsmith raised this point on https://reviews.llvm.org/D111548). If we do decide to make this an error by default, I'd like to do this in a followup change if possible, as a) this change is already a strict improvement over the status quo; b) adding a command-line flag with associated tests would further increase the scope of this change, and c) this change is a blocker for https://reviews.llvm.org/D111548, which others on my team are waiting to be able to use.
> > ... but there may be compatibility concerns for other attributes. This may mean that we can never reduce this list to nothing, but we should try.
>
> I would be pretty aggressive there and break compatibility over this, unless the uses were in system headers or something where the break would be too onerous to justify.
>
> > I think the other question here is whether, by default, this should be an error, not a warning, but this would then presumably require a command-line flag to downgrade the error to a warning if needed
>
> You put `DefaultError` onto the warning diagnostic to have it automatically upgraded as if the user did `-Werror=whatever-warning`, which allows the user to downgrade it back into a warning with `-Wno-error=whatever-warning`.
> > I think the other question here is whether, by default, this should be an error, not a warning, but this would then presumably require a command-line flag to downgrade the error to a warning if needed
>
> You put `DefaultError` onto the warning diagnostic to have it automatically upgraded as if the user did `-Werror=whatever-warning`, which allows the user to downgrade it back into a warning with `-Wno-error=whatever-warning`.
Oh nice -- I didn't realize it was this easy!
I haven't made a change yet because I wanted to give @rsmith an opportunity to weigh in -- do you have an opinion on whether this should be a warning or an error by default?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8325-8326
+ // ParsedAttr::isCXX11Attribute() as the latter includes the alignas
+ // attribute. The alignas attribute is a declaration attribute but can appear
+ // on the `DeclSpec`, so we want to let it through here.
+ if (AL.getSyntax() == ParsedAttr::AS_CXX11 && !Options.IncludeCXX11Attributes)
----------------
rsmith wrote:
> I'm not following what's going on here: the C++11 `alignas` keyword follows the same rules as a C++11-syntax declaration attribute, so I'd expect it to be handled the same way. Eg, `int alignas(16) n;` is ill-formed (even though ICC and MSVC allow it and GCC only warns).
My previous comment was based merely on the fact that I sometimes observed the attribute on the `DeclSpec`, but I did not dig any further at the time. I should have because I would have discovered the actual reason, namely that `isCXX11Attribute()` erroneously classifies the `_Alignas` spelling of the attribute as a C++11 attribute. See the changed comment above and the comment in `isAlignasAttribute()` for details.
I'm not sure how `isAlignasAttribute()` should best be fixed, so I would propose leaving the current logic in place for the time being as a workaround.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8381
+ if (AL.slidesFromDeclToDeclSpec() &&
+ (clang::isa<DeclaratorDecl>(D) || clang::isa<TypeAliasDecl>(D))) {
+ // Suggest moving the attribute to the type instead, but only for our
----------------
aaron.ballman wrote:
> `isa` is variadic.
Nice. Done!
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385
+ // portability.
+ if (AL.isClangScope()) {
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
aaron.ballman wrote:
> I do wonder how much code we break if we tighten this up regardless of which vendor namespace the attribute belongs to. I believe the sliding behavior is not conforming behavior unless we issue a diagnostic about it because we're extending the behavior of the language.
> I do wonder how much code we break if we tighten this up regardless of which vendor namespace the attribute belongs to.
I think this is unfortunately hard to determine?
> I believe the sliding behavior is not conforming behavior unless we issue a diagnostic about it because we're extending the behavior of the language.
It might not conform with the spirit of the standard, but I think it conforms with the letter. All of the attributes in question are vendor extensions anyway, so if we want to, we can define their behavior as “when applied to a declaration, changes the type of the declared entity in a certain way”. This is essentially what all of the `DeclOrTypeAttr` attributes do when applied to a declaration.
And GCC certainly allows putting some of these attributes on declarations. Here’s an example for `vector_size`:
https://godbolt.org/z/1nhjPs3PP
Maybe this means that `vector_size` should actually be a `DeclOrTypeAttr`, not a pure `TypeAttr`. But I’d like to defer that decision to a potential later cleanup dedicated to `vector_size` and leave it as a `TypeAttr` for now.
================
Comment at: clang/test/Parser/MicrosoftExtensions.cpp:65
+ // Can't apply a UUID to a using declaration.
+ [uuid("000000A0-0000-0000-C000-00000000004A")] using base::a; // expected-error {{expected member name}}
+};
----------------
rsmith wrote:
> This seems like a confusing diagnostic to produce for this error. Does it make sense in context (with the snippet + caret)?
The error with snippet and caret looks like this:
```
MicrosoftExtensions.cpp:66:50: error: expected member name or ';' after declaration specifiers
[uuid("000000A0-0000-0000-C000-00000000004A")] using base::a; // expected-error {{expected member name}}
^
```
I would classify that as "not great, not terrible". I think it's defensible given that a) I think it's an error few people are likely to make, so few people will see the error message, and b) MSVC's error message isn't great either:
```
<source>(2): error C2059: syntax error: 'using'
<source>(2): error C2238: unexpected token(s) preceding ';'
```
(see https://godbolt.org/z/jrx6Y1Y83)
If we wanted a better error message, we'd have to do the `MaybeParseMicrosoftAttributes()` somewhere above ParseDeclCXX.cpp:2691, then add a `ProhibitAttributes()` inside the `if (Tok.is(tok::kw_using))`. I'm not sure if this is ultimately worth it though?
================
Comment at: clang/test/SemaCXX/address-space-placement.cpp:3-5
+// Check that we emit the correct warnings in various situations where the C++11
+// spelling of the `address_space` attribute is applied to a declaration instead
+// of a type.
----------------
rsmith wrote:
> I'd like to see some matching tests that we *don't* warn when the attribute is placed properly.
Good point. I've added those tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126061/new/
https://reviews.llvm.org/D126061
More information about the cfe-commits
mailing list