[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 20 18:17:04 PDT 2022
rsmith added a comment.
This direction looks good to me. Thanks!
Regarding the possibility of storing the declaration attributes on the `DeclSpec` rather than on the `Declarator`, I wonder if a better choice might be to make the `Declarator` hold a non-owning reference to the attributes, like it does for the `DeclSpec`?
================
Comment at: clang/include/clang/Sema/DeclSpec.h:1975-1984
/// Reset the contents of this Declarator.
void clear() {
+ clearExceptDeclarationAttrs();
+ DeclarationAttrs.clear();
+ }
+
+ /// Reset the contents of this Declarator, except for the declaration
----------------
Note that neither of these functions clear the `DeclSpec`. I suppose that makes sense given that the `Declarator` does not own the `DeclSpec` but merely holds a reference to it. Perhaps we could do the same thing with the declaration 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,
----------------
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.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:1832
// Parse the common declaration-specifiers piece.
ParsingDeclSpec DS(*this);
----------------
Ideally I think we should pass the `DeclSpecAttrs` into `DS` up-front here. That'd keep the behavior closer to the behavior we get for attributes we parse in the `ParseDeclarationSpecifiers` call below, and will keep attributes in the correct order in the case of something like `__attribute__((a)) int __attribute__((b)) x;` at block scope.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:2188
// Parse the next declarator.
- D.clear();
+ D.clearExceptDeclarationAttrs();
D.setCommaLoc(CommaLoc);
----------------
I wonder if a name like `prepareForNextDeclarator` or `clearForComma` would be better here -- something that indicates why we're clearing rather than describing how.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4385
+ // can put them on the next field.
+ Attrs.takeAllFrom(DeclaratorInfo.D.getDeclarationAttributes());
}
----------------
I think we'd benefit here from the `Declarator` only holding a reference to the declaration attributes rather than owning them.
================
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.
----------------
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).
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/ParseObjc.cpp:661
allTUVariables.push_back(
- ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs));
+ ParseDeclaration(DeclaratorContext::File, DeclEnd, attrs, attrs));
continue;
----------------
It's a bit confusing to use the same `attrs` variable twice here. Would be clearer if either `attrs` were renamed to `NoAttrs` or if you used two different variables. And... actually, given that `ParseDeclaration` takes *mutable* references to its attribute lists, I'm pretty concerned by passing in two different mutable references to the same variable -- that seems error-prone.
But see comments elsewhere; I think we can remove the `DeclSpecAttrs` parameter from `ParseDeclaration` entirely.
================
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());
----------------
I think we should keep the attributes in appearance order.
================
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);
----------------
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.
================
Comment at: clang/lib/Parse/Parser.cpp:743
///
+/// The `Attrs that are passed in are C++11 attributes and appertain to the
+/// declaration.
----------------
error: unmatched `
================
Comment at: clang/lib/Parse/Parser.cpp:935-937
+ ParsedAttributes DeclSpecAttrs(AttrFactory);
+ return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
+ DeclSpecAttrs);
----------------
For consistency with the below cases, call this one `EmptyDeclSpecAttrs` too
================
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);
----------------
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.)
================
Comment at: clang/lib/Sema/ParsedAttr.cpp:219
+ if (!isStandardAttributeSyntax())
+ return true;
+
----------------
I think this case is unreachable, because only the standard `[[...]]` syntax attributes are parsed as declaration attributes.
================
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);
----------------
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();
}
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8361
+ break;
+ if (AL.slidesFromDeclToDeclSpec()) {
+ if (AL.isStandardAttributeSyntax() && AL.isClangScope()) {
----------------
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;
```
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118
+ ProcessDeclAttributeOptions Options;
+ Options.IncludeCXX11Attributes = AL.isCXX11Attribute();
+ ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options);
----------------
This seems to be equivalent to setting `IncludeCXX11Attributes` to `true`, which seems to be equivalent to not setting it at all.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9152-9153
void Sema::checkUnusedDeclAttributes(Declarator &D) {
::checkUnusedDeclAttributes(*this, D.getDeclSpec().getAttributes());
+ ::checkUnusedDeclAttributes(*this, D.getDeclarationAttributes());
::checkUnusedDeclAttributes(*this, D.getAttributes());
----------------
May as well process these in lexical order.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9265
+ S, D, PD.getDeclSpec().getAttributes(),
+ ProcessDeclAttributeOptions().WithIgnoreTypeAttributes(true));
+ }
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9283-9289
+ ParsedAttributesView NonSlidingAttrs;
+ for (ParsedAttr &AL : PD.getDeclarationAttributes()) {
+ if (!AL.slidesFromDeclToDeclSpec()) {
+ NonSlidingAttrs.addAtEnd(&AL);
+ }
+ }
+ ProcessDeclAttributeList(S, D, NonSlidingAttrs);
----------------
These attributes should be processed first, so that we handle attributes in appearance order.
================
Comment at: clang/lib/Sema/SemaType.cpp:201
+ // Set to indicate that, if we're currently processing the DeclSpec, the
+ // attributes we're seeing were actually written ahead of the declaration.
+ bool isProcessingDeclarationAttrs = false;
----------------
It's not immediately clear to me which side is "ahead of"; "before" or "after" would be clearer.
================
Comment at: clang/lib/Sema/SemaType.cpp:593
state.addIgnoredTypeAttr(attr);
-}
+ }
----------------
Phabricator shows this line as slightly over-indented, but that might just be phabricator being weird?
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