[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

This is a bold direction but I like it a lot. Over to @aaron.ballman for final approval.



================
Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+  switch (getParsedKind()) {
+  case AT_AddressSpace:
+  case AT_OpenCLPrivateAddressSpace:
----------------
mboehme wrote:
> 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.
> 
> 
Thanks for the explanation. If we're landing both patches more or less together anyway, I'm not too concerned by how we split the changes between them. No action required here, I think.


================
Comment at: clang/lib/Sema/ParsedAttr.cpp:241
+  case AT_ObjCGC:
+  case AT_VectorSize:
+    return true;
----------------
mboehme wrote:
> 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.
> https://godbolt.org/z/Ke1E7TnYe

That GCC behavior is shocking. Shocking enough that I think the following approach would make sense:

1) For compatibility, emulate the GCC behavior as much as possible for `[[gnu::vector_size]]`, except:
    - Continue to error rather than only warning in the cases where the attribute does not create a vector type at all, and
    - Warn on cases like `int *[[gnu::vector_size(N)]]` (and possibly `int *p [[gnu::vector_size(N)]]`) that we're forming a pointer to a vector rather than a vector of pointers.
2) Add a separate `[[clang::vector_size(N)]]` attribute that behaves like a type attribute is supposed to: the attribute applies to the type on its left, always (and can't be applied to a declaration).

(Incidentally, I think we should add a `[[clang::ext_vector_type]]` spelling with behavior (2) too. The .td file claim that ext_vector_type is OpenCL-specific isn't true; it's a general kind of vector type that follows the OpenCL rules but is available across language modes.)

But... not in this patch. In this patch I think the only priority for the `[[gnu::...]]` attributes is that we don't make things worse (in particular, don't make GCC compatibility worse). And given that this new sliding behavior applies only to `[[clang::...]]` attributes I think we have that covered. So no action required here.


================
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:
> > > 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?
> I'm not certain if @rsmith has opinions here or not, but I think an error which can be downgraded to a warning is a reasonable approach. We should hear pretty quickly if this causes significant problems (in system headers or the like).
I think I'd prefer for this to be a warning in the initial release with the new behavior, and for us to upgrade it to an error for a subsequent release. I'm not completely comfortable with there being no `[[clang::...]]` placement for these attributes that is accepted by default in both Clang 14 and Clang 15, but somewhat more comfortable with there being no syntax that's accepted in both Clang 14 and Clang 16 by default, even though I think these attributes are in practice mostly used with `__attribute__((...))` syntax.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061