[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
Thu Jun 9 06:32:29 PDT 2022
mboehme marked an inline comment as done.
mboehme added inline comments.
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+ switch (getParsedKind()) {
+ case AT_AddressSpace:
+ case AT_OpenCLPrivateAddressSpace:
----------------
rsmith wrote:
> 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.
> 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.
Yes, and we get the same behavior for all C++11 attributes.
This is because, today, Clang flat-out rejects _all_ C++11 attributes that are placed after a decl-specifier-seq:
>From `ParseDeclarationSpecifiers()`:
```
// Reject C++11 attributes that appertain to decl specifiers as
// we don't support any C++11 attributes that appertain to decl
// specifiers. This also conforms to what g++ 4.8 is doing.
ProhibitCXX11Attributes(attrs, diag::err_attribute_not_type_attr);
```
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L3182
So really what the "slides" terminology means is "is treated as appertaining to the decl-specifier-seq instead of the declaration".
> Hm, but we accept:
>
> ```
> int *[[clang::address_space(5)]] *z;
> ```
>
> ... so it really *does* seem to behave like a type attribute sometimes?
Yes, because Clang does allow C++11 attributes (including `address_space` in particular) on declarator chunks today -- it just doesn't allow them on the decl-specifier-seq. As an aside, this makes the error message we get when putting the attribute on the decl-specifier-seq today ("'address_space' attribute cannot be applied to types") pretty misleading.
> 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.
Actually, 1) happens in https://reviews.llvm.org/D111548, which this change is based on.
I did things this way because
- I want to introduce `annotate_type` first, so that this change can use it in tests as an example of a type attribute that doesn't "slide"
- I wanted tests in https://reviews.llvm.org/D111548 to be able to use `annotate_type` in all places that it's intended for, including on the decl-specifier-seq, so I needed to open up that usage. (Actually, when I first authored https://reviews.llvm.org/D111548, I didn't even realize that this followup change would be coming.)
Is this confusing? I could instead do the following:
- Move the change that allows C++11 attributes on the decl-specifier-seq from https://reviews.llvm.org/D111548 to here.
- Move tests that use `annotate_type` on the decl-specifier-seq from https://reviews.llvm.org/D111548 to here.
Do you think that would be preferable?
> 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.
That's true -- and I think it's the best we can do. The behavior that Clang exhibits today is of course set in stone (error out if the attribute is placed on the decl-specifier-seq), and at the same time it seems obvious that we want to nudge people away from putting the attribute on the declaration, as it's really a type attribute. (Just repeating your analysis here essentially; I think we agree, and there's nothing to do here.)
I do think this makes an argument that the new diagnostic we're introducing should be a warning initially, not an error. Otherwise, there would be no way to use the C++11 spelling in a way that compiles (albeit maybe with warnings) both before and after this patch.
> 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;
----------------
rsmith wrote:
> 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?
> 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
Actually, for z, Clang gives me not just a warning but also an error:
```
<source>:1:8: warning: GCC does not allow the 'vector_size' attribute to be written on a type [-Wgcc-compat]
int *[[gnu::vector_size(8)]]* z;
^
<source>:1:8: error: invalid vector element type 'int *'
```
https://godbolt.org/z/E4ss8rzWE
> 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?
Thanks for pointing this out to me!
Actually, I must not have checked the GCC behavior previously, so I went too far and gave `vector_size` meaning when placed on `y` instead of just ignoring it. I’ve now changed the code to ignore the attribute and emit a warning, like GCC does. (See tests in `vector-gcc-compat.c`.)
It’s particularly surprising to me that GCC allows `vector_size` to be applied to a pointer type (the `z` case) – particularly so since the [documentation](https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html) explicitly states: “The vector_size attribute is only applicable to integral and floating scalars, although arrays, pointers, and function return values are allowed in conjunction with this construct.”
I wanted to test whether GCC just silently ignores the attribute when placed on a pointer or whether it has an effect. Here are some test cases:
https://godbolt.org/z/Ke1E7TnYe
>From looking at the generated code, it appears that `vector_size`, when applied to a pointer type, _does_ have an effect – though a maybe slightly surprising one: It is applied to the base type, rather than the pointer type. I suspect that GCC simply treats the C++11 spelling the same as the `__attribute__` spelling (i.e. it doesn’t apply the stricter C++11 rules on what the attribute should appertain to), and it seems that both spellings get applied to the base type of the declaration no matter where they appear in the declaration (except if the attribute is ignored entirely).
Interestingly, this means that Clang’s warning (“GCC does not allow the 'vector_size' attribute to be written on a type”) is wrong. Maybe this is behavior in GCC that has changed since the warning was introduced?
Given all of this, I propose we treat the various cases as follows (and I’ve implemented this in the code):
* We continue to accept `x` without a warning, just as we do today (same behavior as GCC)
* We allow `y` but warn that the attribute doesn’t have an effect (same behavior as GCC)
* We continue to reject `z`, even though GCC allows it and it has an effect there. Rationale: a) We reject this today, so this isn’t a regression; b) this usage is unusual and likely not to occur often in the wild; c) fixing this to be consistent with GCC will take a non-trivial amount of code, so I’d like to keep it outside the scope of this change.
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243
+ case AT_Regparm:
+ case AT_NoDeref:
+ case AT_ObjCGC:
+ case AT_VectorSize:
----------------
rsmith wrote:
> 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?
> 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.
[snip]
Thanks for the context!
> 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.
Agree. In case it was unclear: I certainly wasn't proposing doing this as part of this patch (not sure if that was unclear) -- this was more of a question about principles / intended direction.
> 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)
Yes, this is the particular example that I found confusing. Good point about there being a tension between this being a declaration attribute syntactically but a type attribute (among other things) semantically.
> 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?
I would also be interested in this, though certainly any changes would be outside the scope of this patch.
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