[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 8 14:11:47 PDT 2022
rsmith added inline comments.
================
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:
> 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?
Yes, no action requested. Thanks for talking through this option with me! :)
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+ switch (getParsedKind()) {
+ case AT_AddressSpace:
+ case AT_OpenCLPrivateAddressSpace:
----------------
I don't think this one really slides today, in the syntactic sense, prior to this patch (and I guess the same is true for most of these). Given:
```
[[clang::address_space(5)]] int x;
int [[clang::address_space(5)]] y;
```
... we accept `x` and reject `y`, making it look like this is simply a declaration attribute. Hm, but we accept:
```
int *[[clang::address_space(5)]] *z;
```
... so it really *does* seem to behave like a type attribute sometimes?
OK, so this patch is doing two things:
1) It's permitting the use of these attributes as type attributes, where they were not consistently permitted previously.
2) It's deprecating / warning on uses of them as declaration attributes.
That means there's no way to write code that is compatible with both old and new Clang and produces no warnings, but it's easy enough to turn the warning flag off I suppose. The new behavior does seem much better than the old.
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:241
+ case AT_ObjCGC:
+ case AT_VectorSize:
+ return true;
----------------
This one is weird. Given:
```
[[gnu::vector_size(8)]] int x;
int [[gnu::vector_size(8)]] y;
int *[[gnu::vector_size(8)]]* z;
```
- Clang and GCC accept `x`
- Clang rejects `y` and GCC warns that the attribute is ignored on `y`
- Clang accepts `z` with a warning that GCC would ignore the attribute, whereas GCC silently accepts
I guess after this patch we'll silently accept `x` (treated as the "sliding" behavior) and accept `y` and `z` with a warning that it's GCC-incompatible?
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243
+ case AT_Regparm:
+ case AT_NoDeref:
+ case AT_ObjCGC:
+ case AT_VectorSize:
----------------
mboehme wrote:
> 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.
> 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?
We historically didn't have type attributes at all, only declaration attributes (and I suspect GCC was once the same in this regard), so a lot of the attributes that notionally *should* be treated as type attributes actually behave syntactically as declaration attributes instead. An obvious example of this is the `aligned` attribute, which notionally produces a different type -- that attribute appertains to `typedef` declarations (so that the typedef-name names a different type than the type that it's a typedef for) rather than appertaining to the type. Logically, I think we *should* provide versions of these attributes that appertain to types instead, but that will require a separate proposal and discussion, and is certainly outside the scope of this change.
There are even cases of declaration attributes that logically *should* be type attributes, and are listed in the .td file as `TypeAttr`, but are actually declaration attributes. `MatrixType` is an example:
- It's given a `SubjectList` in the .td file that contains only `TypedefName` because it can syntactically appertain only to a `TypedefNameDecl` -- it's syntactically a declaration attribute, not a type attribute
- It's modeled in the .td file as a `TypeAttr`, possibly because it gets semantically modeled as part of an `AttributedType` -- it's semantically *both* a declaration attribute (we find it on `TypedefNameDecl`s) and a type attribute (we find it on `AttributedType`s)
I'm not entirely convinced that's the right way to model this in the .td file -- the documentation for `TypeAttr` certainly suggests that it's about the syntactic form. Maybe there's room for improvement there -- @aaron.ballman, what do you think?
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