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

Tue Jun 14 05:05:24 PDT 2022

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Only thing left for me are the nits I pointed out in the last review, otherwise I think this is all set. Giving my LG because I don't think we need another round of review for those nits unless you want it

================
Comment at: clang/lib/Sema/ParsedAttr.cpp:241
+  case AT_ObjCGC:
+  case AT_VectorSize:
+    return true;
----------------
rsmith wrote:
> 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.
+1 to this :-)

================
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()) {
----------------
rsmith wrote:
> 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.
Okay, that's convincing, thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126061