[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
Tue May 24 10:12:51 PDT 2022
aaron.ballman added a comment.
Thank you for your work on this, I generally like this new approach and think we're heading in the right direction.
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
/// Attrs - Attributes.
ParsedAttributes Attrs;
+ ParsedAttributes DeclarationAttrs;
----------------
We should probably rename `Attrs` to be less generic and add some comments to explain why there are two lists of parsed attributes.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:657-658
+ /// "slides" to the decl-specifier-seq).
+ /// Attributes with GNU, __declspec or keyword syntax generally slide
+ /// to the decl-specifier-seq. C++11 attributes specified ahead of the
+ /// declaration always appertain to the declaration according to the standard,
----------------
rsmith wrote:
> I don't think this sentence is correct: "Attributes with GNU, __declspec or keyword syntax generally slide to the decl-specifier-seq." Instead, I think those attribute syntaxes are never parsed as declaration attributes in the first place, so there is no possibility of "sliding" anywhere -- they simply always are decl-spec attributes.
That's fair -- I tend to think "sliding" is also what happens (at least notionally) in a case like `__attribute__((address_space(1))) int *i;` because it's a type attribute rather than a declaration attribute, but that's also a declaration specifier, so it doesn't really slide anywhere.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2188
// Parse the next declarator.
- D.clear();
+ D.clearExceptDeclarationAttrs();
D.setCommaLoc(CommaLoc);
----------------
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).
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4323-4326
+ // C2x draft 6.7.2.1/9 : "The optional attribute specifier sequence in a
+ // member declaration appertains to each of the members declared by the
+ // member declarator list; it shall not appear if the optional member
+ // declarator list is omitted."
----------------
Good catch! This also applies in C++: http://eel.is/c++draft/class#mem.general-14
I think you should add some test coverage for this, along the lines of:
```
struct S {
[[clang::annotate("test")]] int; // The attribute should be diagnosed (as an error?)
};
```
================
Comment at: clang/lib/Parse/ParseDecl.cpp:6978-6997
// Parse any C++11 attributes.
- MaybeParseCXX11Attributes(DS.getAttributes());
+ ParsedAttributes ArgDeclAttrs(AttrFactory);
+ MaybeParseCXX11Attributes(ArgDeclAttrs);
- // Skip any Microsoft attributes before a param.
- MaybeParseMicrosoftAttributes(DS.getAttributes());
-
- SourceLocation DSStart = Tok.getLocation();
+ ParsedAttributes ArgDeclSpecAttrs(AttrFactory);
// If the caller parsed attributes for the first argument, add them now.
----------------
rsmith wrote:
> Seems to be mostly pre-existing, but I don't think this is right. The `FirstArgAttrs` are all decl-specifier attributes (they're parsed in `ParseParenDeclarator`, where we look for GNU attributes and `__declspec` attributes), so if we parsed any of those, we should not now parse any syntax that is supposed to precede decl-specifier attributes. The current code allows attributes to appear in the wrong order in the case where we need to disambiguate a paren declarator: https://godbolt.org/z/bzK6n8obM (note that the `g` case properly errors, but the `f` case that needs lookahead to determine whether the `(` is introducing a function declarator incorrectly accepts misplaced attributes).
Good catch!
================
Comment at: clang/lib/Parse/ParseObjc.cpp:1233-1234
// Now actually move the attributes over.
takeDeclAttributes(attrs, D.getMutableDeclSpec().getAttributes());
+ takeDeclAttributes(attrs, D.getDeclarationAttributes());
takeDeclAttributes(attrs, D.getAttributes());
----------------
rsmith wrote:
> I think we should keep the attributes in appearance order.
+1
FWIW, we run into some "fun" bugs with attribute orders and declaration merging where the orders are opposite and it causes problems as in https://godbolt.org/z/58jTM4sGM (note how the error and the note swap positions), so the order of attributes tends to be important (both for semantic behavior as well as diagnostic behavior).
================
Comment at: clang/lib/Parse/ParseStmt.cpp:198
+ ParsedAttributes Attrs(AttrFactory);
+ ConcatenateAttributes(CXX11Attrs, GNUAttrs, Attrs);
+
----------------
I *think* this is going to be okay because attributes at the start of a statement have a very specific ordering, but one thing I was slightly worried about is attribute orders getting mixed up if the caller used something like `ParseAttributes()` or `MaybeParseAttributes()` where you can interleave the syntaxes.
If this turns out to be a real issue at some point, I suppose we could have `ConcatenateAttributes()` do insertions in sort order based on source locations, but I think it'd be best to avoid that unless we see a real issue.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:242-246
+ Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+ GNUAttrs, &GNUAttributeLoc);
} else {
- Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, Attrs);
+ Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+ GNUAttrs);
----------------
rsmith wrote:
> I think this is the only place where we're passing decl-specifier-seq attributes into `ParseDeclaration`. There are only two possible cases here:
>
> 1) We have a simple-declaration, and can `ParseSimpleDeclaration` directly.
> 2) We have a static-assert, using, or namespace alias declaration, which don't permit attributes at all.
>
> So I think we *could* simplify this so that decl-spec attributes are never passed into `ParseDeclaration`:
>
> * If the next token is `kw_static_assert`, `kw_using`, or `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> * Otherwise, call `ParseSimpleDeclaration` and pass in the attributes.
> * Remove the `DeclSpecAttrs` list from `ParseDeclaration`.
>
> I'm not requesting a change here -- I'm not sure whether that's a net improvement or not -- but it seems worth considering.
I think this is a good avenue to explore -- passing in two different attribute lists means someone will at some point get it wrong by accident, so only having one attribute list reduces the chances for bugs later. I don't imagine static assertions or namespaces will get leading attributes. However...
I think asm-declaration and using-directive are also a bit special -- they're allowed to have leading attributes: http://eel.is/c++draft/dcl.dcl#nt:asm-declaration and http://eel.is/c++draft/dcl.dcl#nt:using-directive
Do we also have to handle opaque-enum-declaration here? http://eel.is/c++draft/dcl.dcl#nt:block-declaration
================
Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
case tok::kw_asm: {
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
bool msAsm = false;
----------------
It seems like we might have a pre-existing bug here for [[]] attributes? http://eel.is/c++draft/dcl.dcl#nt:asm-declaration
================
Comment at: clang/lib/Parse/ParseStmt.cpp:329-330
case tok::kw___if_not_exists:
- ProhibitAttributes(Attrs);
+ ProhibitAttributes(CXX11Attrs);
+ ProhibitAttributes(GNUAttrs);
ParseMicrosoftIfExistsStatement(Stmts);
----------------
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.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:340
case tok::kw___try:
- ProhibitAttributes(Attrs); // TODO: is it correct?
+ ProhibitAttributes(CXX11Attrs); // TODO: is it correct?
+ ProhibitAttributes(GNUAttrs);
----------------
You can remove the TODO, it is correct: https://godbolt.org/z/PEcarx1nr
================
Comment at: clang/lib/Parse/Parser.cpp:1164
DS.getParsedSpecifiers() == DeclSpec::PQ_StorageClassSpecifier) {
- Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File);
+ Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File, Attrs);
return Actions.ConvertDeclToDeclGroup(TheDecl);
----------------
rsmith wrote:
> We should `ProhibitAttrs` here rather than passing them on.
> ```
> [[]] extern "C" void f();
> ```
> ... is invalid. (Per the grammar in https://eel.is/c++draft/dcl.dcl#dcl.pre-1 and https://eel.is/c++draft/dcl.dcl#dcl.link-2 an attribute-specifier-seq can't appear here.)
+1, looks like we're missing test coverage for that case (but those diagnostics by GCC or MSVC... hoo boy!): https://godbolt.org/z/cTfPbK837
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:219
+ if (!isStandardAttributeSyntax())
+ return true;
+
----------------
rsmith wrote:
> I think this case is unreachable, because only the standard `[[...]]` syntax attributes are parsed as declaration attributes.
I think it's unreachable today, but there's a nonzero chance for it to become reachable in the near future as WG21 and WG14 continue to contemplate adding standardized type attributes, so it seems reasonably forward-thinking to have 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()) {
----------------
Thoughts on the additional comment? And should we start to issue that deprecation warning now (perhaps as a separate follow-up commit)?
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:306-307
+ ParsedAttributes &Result) {
+ // Note that takeAllFrom() puts the attributes at the beginning of the list,
+ // so to obtain the correct ordering, we add `Second`, then `First`.
+ Result.takeAllFrom(Second);
----------------
rsmith wrote:
> Are you sure about this? I looked at the implementation of `takeAllFrom` and `takePool`, and it looks like it adds the new attributes to the end:
> ```
> void AttributePool::takePool(AttributePool &pool) {
> Attrs.insert(Attrs.end(), pool.Attrs.begin(), pool.Attrs.end());
> pool.Attrs.clear();
> }
> ```
As mentioned earlier, we've got some preexisting ordering confusion somewhere in our attribute processing code, so I wouldn't be surprised if we're getting close to finding the root cause of that.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8361
+ break;
+ if (AL.slidesFromDeclToDeclSpec()) {
+ if (AL.isStandardAttributeSyntax() && AL.isClangScope()) {
----------------
rsmith wrote:
> Should we also be checking here that `D` is a declaration that the attribute could have been moved onto -- that is, that it's a `DeclaratorDecl`? Presumably we should error rather than only warning on
> ```
> namespace N {}
> [[clang::noderef]] using namespace N;
> ```
We have `ParsedAttr::diagAppertainsToDecl()`, but that produces a diagnostic about the attribute not applying to the declaration which we wouldn't want. However, if we really wanted this, we could modify ClangAttrEmitter.cpp to produce a helper function for testing appertainment.
================
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();
+ }
----------------
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?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118
+ ProcessDeclAttributeOptions Options;
+ Options.IncludeCXX11Attributes = AL.isCXX11Attribute();
+ ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options);
----------------
rsmith wrote:
> This seems to be equivalent to setting `IncludeCXX11Attributes` to `true`, which seems to be equivalent to not setting it at all.
Hmmm, not quite -- `AL.isCXX11Attribute()` may return `false` (like for the GNU spelling of this attribute).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9265
+ S, D, PD.getDeclSpec().getAttributes(),
+ ProcessDeclAttributeOptions().WithIgnoreTypeAttributes(true));
+ }
----------------
rsmith wrote:
> I think we should be skipping C++11 attributes here too: `int [[x]] a;` should not apply the attribute `x` to `a`, even if it's a declaration attribute.
+1 to that example being something we want to diagnose.
================
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)
----------------
Same question here as above on whether we can produce a fix-it here.
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