[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 13 08:07:28 PDT 2022
aaron.ballman added a comment.
In D126061#3577054 <https://reviews.llvm.org/D126061#3577054>, @mboehme wrote:
> @aaron.ballman Any further comments from you on this change?
Mostly just nits from me at this point, plus some answers to questions I had missed previously.
> There is some breakage in the pre-merge checks, but it looks as if it's pre-existing breakage. See the comment-only change here that looks as if it exhibits similar breakage:
>
> https://reviews.llvm.org/D127494
I agree that the precommit CI failures look unrelated to this patch.
================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
// FIXME: Use a better mechanism to determine this.
+ // We use this in `isCXX11Attribute` below, so it _should_ only return
+ // true for the `alignas` spelling, but it currently also returns true
----------------
Now that C23 has attributes, it's also not clear whether `_Alignas` will be handled slightly differently in the future as an attribute rather than a declaration specifier as happened with `_Noreturn` for C23. So agreed this is a tricky area.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1916
+ /// declaration, i.e. `x` and `y` in this case.
+ Declarator(const DeclSpec &ds, const ParsedAttributesView &DeclarationAttrs,
+ DeclaratorContext C)
----------------
I know it's pre-existing code, but since you're updating the parameters and adding some great comments, can you rename `ds` to `DS` per coding style guides?
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1928-1930
+ assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) {
+ return AL.isStandardAttributeSyntax();
+ }));
----------------
Can you add an `&& "and this is what the assert is testing for"` message here so when the assertion fails there's more details as to what went sideways?
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and `Second`.
+void ConcatenateAttributes(ParsedAttributes &First, ParsedAttributes &Second,
+ ParsedAttributes &Result);
----------------
I think `Concatenate` implies that `First` and `Second` are untouched, but that's not really the case here. Perhaps `takeAndConcatenateAll()` or something along those lines instead? Also, the function should start with a lowercase letter per the usual coding style rules.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+ /// This may only be called if isStandardAttributeSyntax() returns true.
+ bool slidesFromDeclToDeclSpec() const;
+
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Because this is now specific to standard attributes, and those don't "slide" under normal circumstances, it might be nice to rename this method to make it more clear that this should not be called normally. I don't have a strong opinion on what a *good* name is, but even something like `improperlySlidesFromDeclToDeclSpec()` would make me happy.
> I've decided to add `LegacyBehavior` at the end -- WDYT?
Works for me
================
Comment at: clang/lib/Parse/ParseStmt.cpp:229
default: {
+ auto isStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); };
if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
----------------
Coding style nit.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
(StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
ParsedStmtContext()) &&
((GNUAttributeLoc.isValid() &&
- !(!Attrs.empty() &&
- llvm::all_of(
- Attrs, [](ParsedAttr &Attr) { return Attr.isStmtAttr(); }))) ||
+ !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+ llvm::all_of(CXX11Attrs, isStmtAttr) &&
+ llvm::all_of(GNUAttrs, isStmtAttr))) ||
----------------
The logic is correct here, but this predicate has gotten really difficult to understand. If you wanted to split some of those conditions out into named variables for clarity, I would not be sad (but I also don't insist either).
================
Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+ if (CXX11Attrs.Range.getBegin().isValid())
+ DeclStart = CXX11Attrs.Range.getBegin();
+ else if (GNUAttrs.Range.getBegin().isValid())
+ DeclStart = GNUAttrs.Range.getBegin();
----------------
Do CXX attributes always precede GNU ones, or do we need to do a source location comparison here to see which location is lexically earlier?
================
Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
case tok::kw_asm: {
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
bool msAsm = false;
----------------
mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > It seems like we might have a pre-existing bug here for [[]] attributes? http://eel.is/c++draft/dcl.dcl#nt:asm-declaration
> > > Yes, we do!
> > >
> > > Fixed, and tests added in test/Parser/asm.c and test/Parser/asm.cpp.
> > I think we should prohibit this for all `[[]]` attributes. C doesn't have an asm statement as part of the standard, but it lists it as a common extension in Annex J and uses the same syntax as the C++ feature. Might as well keep it consistent between the languages.
> My rationale for handling C and C++ differently here was because they treat `asm` differently.
>
> In C++, the standard requires us to allow attributes, so we merely warn that they will be ignored. (Also, `asm` is formally a declaration, not a statement in C++, though I’m not sure that makes a difference to the syntax.)
>
> In C, J.5.10 in “Common extensions” says “The most common implementation is via a statement [...]”. C permits attributes only on declarations, though since `asm` is an extension, that’s not really relevant anyway. Since this is our extension and we wouldn’t do anything with the attributes anyway, it seemed best to me to prohibit them outright.
>
> OTOH, I’ve just checked and GCC appears to accept attributes in this position even in C mode. Is the compatibility argument strong enough that we should do this too?
> In C, J.5.10 in “Common extensions” says “The most common implementation is via a statement [...]”. C permits attributes only on declarations, though since asm is an extension, that’s not really relevant anyway. Since this is our extension and we wouldn’t do anything with the attributes anyway, it seemed best to me to prohibit them outright.
C permits attributes on statements as well. I think it would be a bit odd to prohibit on assembly statements but still allow on other kinds of extensions like GNU ranged case labels.
I think we should go ahead and accept despite it being a bit useless.
================
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:
> > 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.
> 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.
This is why we have `DeclOrTypeAttr` in Attr.td -- largely for the calling convention attributes. It's a newer construct and I don't think it's been applied everywhere it should (it sounds like `MatrixType` is a good example).
> 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?
There is quite a bit of room for improvement there. IIRC, we also got away with a lot of laxity here because automated checking for type attributes is basically nonexistent. However, I think those cleanups can all happen separately from this patch.
================
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()) {
----------------
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).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8387
+ if (AL.slidesFromDeclToDeclSpecLegacyBehavior() &&
+ (clang::isa<DeclaratorDecl, TypeAliasDecl>(D))) {
+ // Suggest moving the attribute to the type instead, but only for our
----------------
This shouldn't need to be qualified and the extra parens aren't needed.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385
+ // portability.
+ if (AL.isClangScope()) {
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
mboehme wrote:
> aaron.ballman wrote:
> > I do wonder how much code we break if we tighten this up regardless of which vendor namespace the attribute belongs to. I believe the sliding behavior is not conforming behavior unless we issue a diagnostic about it because we're extending the behavior of the language.
> > I do wonder how much code we break if we tighten this up regardless of which vendor namespace the attribute belongs to.
>
> I think this is unfortunately hard to determine?
>
> > I believe the sliding behavior is not conforming behavior unless we issue a diagnostic about it because we're extending the behavior of the language.
>
> It might not conform with the spirit of the standard, but I think it conforms with the letter. All of the attributes in question are vendor extensions anyway, so if we want to, we can define their behavior as “when applied to a declaration, changes the type of the declared entity in a certain way”. This is essentially what all of the `DeclOrTypeAttr` attributes do when applied to a declaration.
>
> And GCC certainly allows putting some of these attributes on declarations. Here’s an example for `vector_size`:
>
> https://godbolt.org/z/1nhjPs3PP
>
> Maybe this means that `vector_size` should actually be a `DeclOrTypeAttr`, not a pure `TypeAttr`. But I’d like to defer that decision to a potential later cleanup dedicated to `vector_size` and leave it as a `TypeAttr` for now.
> It might not conform with the spirit of the standard, but I think it conforms with the letter. All of the attributes in question are vendor extensions anyway, so if we want to, we can define their behavior as “when applied to a declaration, changes the type of the declared entity in a certain way”. This is essentially what all of the DeclOrTypeAttr attributes do when applied to a declaration.
The goal is for us to eventually conform to the spirit of the standard. Yes, vendor attributes have a lot of latitude in terms of their semantics, but the reason why we needed to devise the `[[]]` syntax in the first place was because the sliding behavior of `__attribute__(())` caused far too many problems in practice and we needed a much tighter notion of appertainment.
> Maybe this means that vector_size should actually be a DeclOrTypeAttr, not a pure TypeAttr. But I’d like to defer that decision to a potential later cleanup dedicated to vector_size and leave it as a TypeAttr for now.
Fine by me, I think that can be cleaned up later.
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