[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 26 07:50:03 PDT 2022
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
/// Attrs - Attributes.
ParsedAttributes Attrs;
+ ParsedAttributes DeclarationAttrs;
----------------
mboehme wrote:
> aaron.ballman wrote:
> > We should probably rename `Attrs` to be less generic and add some comments to explain why there are two lists of parsed attributes.
> I've added some comments to clarify the difference. I'm not sure what a good alternative name for `Attrs` would be though. The only obvious candidate that comes to mind is `DeclaratorAttrs`, but that seems pretty redundant given that this is a field of `Declarator`.
>
> I think there is actually a good case for calling the first list simply `Attrs` because, unlike `DeclarationAttrs`, it applies directly to the `Declarator`. This distinction is amplified by the fact that, since your review, I've changed `DeclarationAttrs` to be a reference, not a by-value field.
The new comment actually makes it less clear for me -- that says the attributes are for the `DeclSpec` but they're actually for the `Declatator` (there's no relationship between `Attrs` and `DS`).
Given that `DeclSpec` uses `Attrs` for its associated attributes, I think it's okay to use `Attrs` here for the ones associated with the `Declarator`, but the comment should be updated in that case.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+ /// This may only be called if isStandardAttributeSyntax() returns true.
+ bool slidesFromDeclToDeclSpec() const;
+
----------------
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.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ ProhibitAttributes(DeclAttrs);
+ ProhibitAttributes(OriginalDeclSpecAttrs);
DeclEnd = Tok.getLocation();
----------------
rsmith wrote:
> This looks wrong to me (both before and after this patch; you've faithfully retained an incorrect behavior). If `DeclSpec` attributes aren't allowed, they should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after this patch, we'll diagnose attributes in a free-standing decl-specifier-seq if we happened to tentatively parse them, not if we happened to parse them in `ParseDeclarationSpecifiers`. For example:
>
> ```
> __attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
> void f() {
> __attribute(()) struct X; // DeclSpecAttrs will contain the attribute specifier, error
> }
> ```
>
> Comparing to GCC's behavior for that example (which GCC silently accepts) and for this one:
>
> ```
> __attribute__((packed)) struct X; // GCC warns that packed has no meaning here, clang produces a high-quality warning.
> void f() {
> __attribute((packed)) struct X; // GCC warns that packed has no meaning here, clang produces a bogus error.
> }
> ```
>
> ... I think the right thing to do is to delete this call (along with `OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently syntactically permitted here, but most (maybe even all?) GNU attributes don't have any meaning in this position.
Good catch!
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2188
// Parse the next declarator.
- D.clear();
+ D.clearExceptDeclarationAttrs();
D.setCommaLoc(CommaLoc);
----------------
mboehme wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would be better here -- something that indicates why we're clearing rather than describing how.
> > Personally, I like `prepareForNextDeclarator()` -- that's nice and clear (to me).
> Now that `Declarator` only holds a reference to the declaration attributes, the additional `clearExceptDeclarationAttrs()` has become unnecessary. With that, I think the original `clear()` name is acceptable?
It's fine by me.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2716
// Otherwise, it must be a using-declaration or an alias-declaration.
return ParseUsingDeclaration(DeclaratorContext::Member, TemplateInfo,
+ UsingLoc, DeclEnd, DeclAttrs, AS);
----------------
mboehme wrote:
> rsmith wrote:
> > Do we need to `ProhibitAttrs(DeclSpecAttrs)` along this code path? For example:
> > ```
> > struct A { int a; };
> > struct B : A { [uuid("1234")] using A::a; };
> > ```
> > ... currently warns (for `-target x86_64-windows`) but I think with this patch we'll silently drop the `uuid` attribute on the floor.
> Good point.
>
> Instead of doing a `ProhibitAttributes()`, I've decided to simply move the `MaybeParseMicrosoftAttributes()` down below this point -- that seemed more straightforward.
>
> I've added a test in test/Parser/MicrosoftExtensions.cpp.
>
> Some considerations:
>
> - With this patch, we've "upgraded" the warning to a hard error. I think this is defensible though because b) the Microsoft compiler also flags this as a hard error (https://godbolt.org/z/jrx6Y1Y83), b) most people will be using Clang to compile code that they compiled with MSVC originally or in parallel, and c) it's an error that I think people are unlikely to make in the first place,
>
> - The error message we get isn't great, but it's not really worse than the Microsoft compiler's error message, and as noted above, this is an error that I think people are unlikely to make.
>
>
I think it's okay for this to have gone from a warning to a hard error, especially given the MSVC compatibility.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:2745-2746
// ptr-operators.
- Declarator D(DS, DeclaratorContext::ConversionId);
+ ParsedAttributes EmptyDeclAttrs(AttrFactory);
+ Declarator D(DS, EmptyDeclAttrs, DeclaratorContext::ConversionId);
ParseDeclaratorInternal(D, /*DirectDeclParser=*/nullptr);
----------------
rsmith wrote:
> Given the number of times this has come up, I wonder if it's worth adding something like:
>
> ```
> class ParsedAttributes {
> public:
> // ...
> static const ParsedAttributes &none() {
> static AttributeFactory Factory;
> static AttributePool Pool(Factory);
> static const ParsedAttributes Attrs(Pool);
> return Attrs;
> }
> // ...
> };
> ```
> (or, better, changing `Declarator` to hold a `const ParsedAttributesView&` rather than a `const ParsedAttributes&`) so that we can write
> ```
> Declarator D(DS, ParsedAttributes::none(), DeclaratorContext::ConversionId);
> ```
>
> Totally optional, though, and I'm happy for this cleanup to happen at some later point in time.
+1 to this being a pretty nice cleanup and it being fine if it happens in a follow-up.
================
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:
> > 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.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:329-330
case tok::kw___if_not_exists:
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
ParseMicrosoftIfExistsStatement(Stmts);
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Seeing this pattern over and over again... I wonder if we want a variadic version of this function so we can call `ProhibitAttribtues(CXX11Attrs, GNUAttrs);` or if that's just overkill.
> I would lean towards keeping it simple. Duplicating the `ProhibitAttributes()` is more verbose, but it's also less surprising.
I don't feel super strongly about it, so this can wait until a follow-up if we want to do anything about it.
================
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:
> > 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`.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8381
+ if (AL.slidesFromDeclToDeclSpec() &&
+ (clang::isa<DeclaratorDecl>(D) || clang::isa<TypeAliasDecl>(D))) {
+ // Suggest moving the attribute to the type instead, but only for our
----------------
`isa` is variadic.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385
+ // portability.
+ if (AL.isClangScope()) {
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8367-8368
+ // attributes might hurt portability.
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
+ << AL << D->getLocation();
+ }
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Do we have enough information at hand that we could produce a fix-it to move the attribute in question to the type position, or is that more work than it's worth?
> I thought about this, but I think it would be non-trivial and would prefer to leave it to a future enhancement.
Fine by me.
================
Comment at: clang/lib/Sema/SemaType.cpp:1826
+ // attributes might hurt portability.
+ if (AL.isStandardAttributeSyntax() && AL.isClangScope()) {
+ S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
mboehme wrote:
> aaron.ballman wrote:
> > Same question here as above on whether we can produce a fix-it here.
> Again, I would prefer to exclude this from the scope of this change.
SGTM!
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