[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed May 25 14:35:39 PDT 2022
rsmith added inline comments.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+ assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) {
+ return AL.isStandardAttributeSyntax();
+ }));
----------------
I think I recall seeing OpenMP pragma attributes being parsed as declaration attributes too. FIXME update this comment after review is complete
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ ProhibitAttributes(DeclAttrs);
+ ProhibitAttributes(OriginalDeclSpecAttrs);
DeclEnd = Tok.getLocation();
----------------
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.
================
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;
----------------
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?
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:16
#include "clang/Basic/Attributes.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/PrettyStackTrace.h"
----------------
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`.
================
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);
----------------
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 :-)
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243
+ case AT_Regparm:
+ case AT_NoDeref:
+ case AT_ObjCGC:
+ case AT_VectorSize:
----------------
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.
================
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)
----------------
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).
================
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}}
+};
----------------
This seems like a confusing diagnostic to produce for this error. Does it make sense in context (with the snippet + caret)?
================
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.
----------------
I'd like to see some matching tests that we *don't* warn when the attribute is placed properly.
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