This is an archive of the discontinued LLVM Phabricator instance.

[clang] Reject non-declaration C++11 attributes on declarations
ClosedPublic

Authored by mboehme on May 20 2022, 4:38 AM.

Details

Summary

For backwards compatiblity, we emit only a warning instead of an error if the
attribute is one of the existing type attributes that we have historically
allowed to "slide" to the DeclSpec just as if it had been specified in GNU
syntax. (We will call these "legacy type attributes" below.)

The high-level changes that achieve this are:

  • We introduce a new field Declarator::DeclarationAttrs (with appropriate accessors) to store C++11 attributes occurring in the attribute-specifier-seq at the beginning of a simple-declaration (and other similar declarations). Previously, these attributes were placed on the DeclSpec, which made it impossible to reconstruct later on whether the attributes had in fact been placed on the decl-specifier-seq or ahead of the declaration.
  • In the parser, we propgate declaration attributes and decl-specifier-seq attributes separately until we can place them in Declarator::DeclarationAttrs or DeclSpec::Attrs, respectively.
  • In ProcessDeclAttributes(), in addition to processing declarator attributes, we now also process the attributes from Declarator::DeclarationAttrs (except if they are legacy type attributes).
  • In ConvertDeclSpecToType(), in addition to processing DeclSpec attributes, we also process any legacy type attributes that occur in Declarator::DeclarationAttrs (and emit a warning).
  • We make ProcessDeclAttribute emit an error if it sees any non-declaration attributes in C++11 syntax, except in the following cases:
    • If it is being called for attributes on a DeclSpec or DeclaratorChunk
    • If the attribute is a legacy type attribute (in which case we only emit a warning)

The standard justifies treating attributes at the beginning of a
simple-declaration and attributes after a declarator-id the same. Here are some
relevant parts of the standard:

  • The attribute-specifier-seq at the beginning of a simple-declaration "appertains to each of the entities declared by the declarators of the init-declarator-list" (https://eel.is/c++draft/dcl.dcl#dcl.pre-3)

The standard contains similar wording to that for a simple-declaration in other
similar types of declarations, for example:

The new behavior is tested both on the newly added type attribute
annotate_type, for which we emit errors, and for the legacy type attribute
address_space (chosen somewhat randomly from the various legacy type
attributes), for which we emit warnings.

Depends On D111548

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mboehme added inline comments.May 25 2022, 5:50 AM
clang/include/clang/Sema/DeclSpec.h
1853–1858

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.

1996–1998

This is a great idea that also helps with other things. It's similar in spirit to the idea that I'd floated in our discussion on https://reviews.llvm.org/D111548 of introducing a Declaration and having Declarator hold a reference to that, but it seemed a bit excessive to introduce a new Declaration class solely for the purpose of holding the declaration attributes. Holding a reference directly to the declaration attributes achieves the same thing, but without having to introduce yet another class. I like it.

One implication of this is that we should only have a const version of getDeclarationAttributes(); a non-const version would be dangerous because we're potentially sharing the attribute list with other declarators. (In fact, this wasn't really sound before either because we were effectively already sharing the attribute list too by moving it around between declarators.) Fortunately, it turns out that all of the places that were using the non-const getDeclarationAttributes() didn't actually need to be manipulating the declaration attribute list. I've added source code comments to call this out where appropriate.

clang/include/clang/Sema/ParsedAttr.h
657–658

Yes, this was inteded to refer to the fact that non-[[]] attributes also notionally slide to the DeclSpec. In the initial version of my patch, it was possible for slidesFromDeclToDeclSpec() to be called on non-[[]] attributes within the default case in ProcessDeclAttribute().

However, I've realized that this is confusing; it's better to restrict this function only to [[]] attributes and to do an explicit check in ProcessDeclAttribute() to see whether we're looking at a [[]] attribute. I've added documentation to this function making it clear that it should only be called on [[]] attributes, and I've also added an assertion to enforce this.

All other places that call slidesFromDeclToDeclSpec() are already guaranteed to only call it on [[]] attributes because those attributes come from Declarator::getDeclarationAttributes(), and that is only ever populated with [[]] attributes. (I've added an assertion to make sure this is and continues to be true.)

clang/lib/Parse/ParseDecl.cpp
1833

Done. Unfortunately this did require adding the OriginalDeclSpecAttrs for the purpose of the ProhibitAttributes() call below. That call can't simply use DS.getAttributes() because ParseDeclarationSpecifiers() might have added attributes to that that are legitimately allowed to be there. For example, here's a case from test/SemaCXX/MicrosoftExtensions.cpp that would otherwise break:

// expected-warning@+1 {{attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration}}
struct D {} __declspec(deprecated);

Interestingly, I believe the attribute ordering was already correct before this change because DeclSpec::takeAttributesFrom() adds attributes at the beginning of the list. (It may even be that it does this to support this very use case.) However, as we discuss elsewhere, this behavior is potentially surprising, and we may want to change it at some point, so it's better not to rely on it in the first place.

2193

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?

4328–4331

Good point. Test added in test/Parser/c2x-attributes.c.

4389

Agree -- this has made this function much more straightforward.

6979–7001

Thank you -- done!

clang/lib/Parse/ParseDeclCXX.cpp
2715

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.
clang/lib/Parse/ParseObjc.cpp
660–662

Good points -- I've used two separate variables.

1239–1240

As it turns out, we can never pass declaration attributes in here anyway -- see the newly added assertion at the top of the function. This is a good thing, because I've eliminated the non-const version of Declarator::getDeclarationAttributes().

clang/lib/Parse/ParseStmt.cpp
199

Thanks for pointing this out. I agree we shouldn't do this sorting unless it turns out to be necessary.

243–248

I investigated this, but I'm not sure it's a net win.

  • If the next token is kw_static_assert, kw_using, or kw_namespace, then prohibit attributes and call ParseDeclaration.

Or if the next token is kw_inline and the token after that is kw_namespace...

I think going down this path would mean duplicating all of the case distinctions in ParseDeclaration() -- and if we add more cases in the future, we'd have to make sure to replicate them here. I think overall it keeps the code simpler to accept the additional DeclSpecAttrs parameter.

Do we also have to handle opaque-enum-declaration here? http://eel.is/c++draft/dcl.dcl#nt:block-declaration

A moot point, probably, but I believe this is handled by ParseEnumSpecifier(), which is called from ParseDeclarationSpecifiers(), so there would be no need to handle it separately.

318–327

Yes, we do!

Fixed, and tests added in test/Parser/asm.c and test/Parser/asm.cpp.

337–339

I would lean towards keeping it simple. Duplicating the ProhibitAttributes() is more verbose, but it's also less surprising.

clang/lib/Parse/Parser.cpp
935–938

Good point, done!

1165

Fixed and added a test in test/Parser/cxx0x-attributes.cpp.

clang/lib/Sema/ParsedAttr.cpp
219

As explained elsewhere, in the first iteration of the change, we used to actually call this with non-[[]] attributes.

I've changed this now, and there's an assertion that this will only ever be called with [[]] attributes.

224

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.

308–309

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();
}

This is the AttributePool, however, and IIUC that only deals with memory management for attributes; the ordering of attributes there should not have any semantic implications I believe.

What's actually relevant for the semantic ordering of attributes is the call to addAll() in takeAllFrom():

void takeAllFrom(ParsedAttributes &Other) {
  assert(&Other != this &&
         "ParsedAttributes can't take attributes from itself");
  addAll(Other.begin(), Other.end());
  Other.clearListOnly();
  pool.takeAllFrom(Other.pool);
}

And addAll() inserts at the beginning of the list:

void addAll(iterator B, iterator E) {
  AttrList.insert(AttrList.begin(), B.I, E.I);
}

The behavior is definitely surprising though. I speculate that it _may_ have been introduced to make ParseSimpleDeclaration() do the right thing (see my comments there).

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.

It's possible that other callers of takeAllFrom() or addAll() are expecting the attributes to get added at the end and are doing the wrong thing because of that. This seems worth investigating further, but I'd prefer to defer that to a followup change.

clang/lib/Sema/SemaDeclAttr.cpp
8365

Good point. I've added a check that we're looking at either a DeclaratorDecl or a TypeAliasDecl. The latter is necessary because we have existing use cases where attributes can be placed on an alias declaration and is interpreted as appertaining to the type; see this example from test/Parser/cxx0x-attributes.cpp:

using V = int; // expected-note {{previous}}
using V [[gnu::vector_size(16)]] = int; // expected-error {{redefinition with different types}}

The various tests in test/SemaCXX/address-space-placement.cpp now reflect our rules for where the attribute exhibits the legacy "sliding" behavior (with a mere warning) and where it is prohibited outright (see the expected-error cases in that test).

However, if we really wanted this, we could modify ClangAttrEmitter.cpp to produce a helper function for testing appertainment.

That's a good point. I think for the time being what we have here is sufficient however.

8371–8372

I thought about this, but I think it would be non-trivial and would prefer to leave it to a future enhancement.

9139

Thanks for pointing this out -- done!

9298

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.

Good point -- done.

I don't think this will ultimately make much of a difference because we shouldn't be putting C++11 declaration attributes on the DeclSpec anyway; there's code in ParseDeclarationSpecifiers() that prohibits that. (Prior to this change, we were prohibiting C++11 attributes on the DeclSpec outright; now, we only forbid non-type attributes, but that does mean we should never see declaration attributes here.) There are existing tests for this, for example in test/Parser/cxx0x-attributes.cpp (search for "attribute cannot be applied to a declaration).

However, it does seem more logical and consistent to exclude C++11 attributes here, so I've made this change.

One effect that this does have is the treatment of unknown C++11 attributes: Because we're excluding them now, we no longer diagnose them here; instead, I've modified processTypeAttrs() to diagnose unknown C++11 attributes on the DeclSpec there. Again, this seems the more logical way to do things: The attributes appertain to the type, so they should be diagnosed as part of processing type attributes.

clang/lib/Sema/SemaType.cpp
201

No longer applicable.

I've reverted the changes I made to TypeProcessingState that this comment applied to. I had originally modified TypeProcessingState::getCurrentAttributes() so that it would return the declaration attributes if we were currently processing those, but it turned out that this was not relevant to any of the use cases that TypeProcessingState::getCurrentAttributes() is used for.

582

This was actually a mis-formatting that clang-format didn't complain about or fix. Fixed.

1814

Again, I would prefer to exclude this from the scope of this change.

mboehme edited the summary of this revision. (Show Details)May 25 2022, 7:30 AM
rsmith added inline comments.May 25 2022, 2:35 PM
clang/include/clang/Sema/DeclSpec.h
1924–1926

I think I recall seeing OpenMP pragma attributes being parsed as declaration attributes too. FIXME update this comment after review is complete

clang/lib/Parse/ParseDecl.cpp
1848

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.

clang/lib/Parse/ParseDeclCXX.cpp
2691–2696

Oh, neat, I think this copy isn't necessary any more, given that we don't give ownership of the DeclAttrs to the ParsingDeclarator any more, and don't mix the declaration attributes and the DeclSpec attributes together any more. We should be able to just use DeclAttrs instead of FnAttrs below now, I think?

clang/lib/Parse/ParseExprCXX.cpp
2745–2747

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.

clang/lib/Parse/ParseStmt.cpp
16

This header is intended to be private to Sema. If you want to use a diagnostic in both the parser and sema, please move it to DiagnosticCommonKinds.td.

243–248
  • If the next token is kw_static_assert, kw_using, or kw_namespace, then prohibit attributes and call ParseDeclaration.

Or if the next token is kw_inline and the token after that is kw_namespace...

This function is parsing block declarations, so we don't have to handle inline namespace here because that can't appear as a block declaration. The only reason we need to handle namespace is to support namespace aliases (namespace A = B;), which are (perhaps surprisingly) allowed at block scope.

I think going down this path would mean duplicating all of the case distinctions in ParseDeclaration() -- and if we add more cases in the future, we'd have to make sure to replicate them here. I think overall it keeps the code simpler to accept the additional DeclSpecAttrs parameter.

It's not all the cases, only the block-declaration cases. If we wanted to directly follow the grammar, we could split a ParseBlockDeclaration function out of ParseDeclaration, but I guess there'd be so little left in ParseDeclaration that it wouldn't be worth it, and we'd still have to pass the DeclSpecAttrs parameter to ParseBlockDeclaration, so that doesn't save us anything. I suppose it'd also regress our diagnostic quality for declarations that aren't allowed at block scope. OK, I'm convinced :-)

clang/lib/Sema/ParsedAttr.cpp
240–243

Many of these were previously not permitted in type attribute position, despite being declared in Attr.td as TypeAttrs. 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.

clang/lib/Sema/SemaDeclAttr.cpp
8325–8326

I'm not following what's going on here: the C++11 alignas keyword follows the same rules as a C++11-syntax declaration attribute, so I'd expect it to be handled the same way. Eg, int alignas(16) n; is ill-formed (even though ICC and MSVC allow it and GCC only warns).

clang/test/Parser/MicrosoftExtensions.cpp
65

This seems like a confusing diagnostic to produce for this error. Does it make sense in context (with the snippet + caret)?

clang/test/SemaCXX/address-space-placement.cpp
4–6

I'd like to see some matching tests that we *don't* warn when the attribute is placed properly.

I'm very happy with how this patch is looking.

clang/include/clang/Sema/DeclSpec.h
1924–1926

Oops, forgot to circle back to this. I think I was wrong; please disregard :)

aaron.ballman added inline comments.May 26 2022, 7:50 AM
clang/include/clang/Sema/DeclSpec.h
1853–1858

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.

clang/include/clang/Sema/ParsedAttr.h
664

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.

clang/lib/Parse/ParseDecl.cpp
1848

Good catch!

2193

It's fine by me.

clang/lib/Parse/ParseDeclCXX.cpp
2715

I think it's okay for this to have gone from a warning to a hard error, especially given the MSVC compatibility.

clang/lib/Parse/ParseExprCXX.cpp
2745–2747

+1 to this being a pretty nice cleanup and it being fine if it happens in a follow-up.

clang/lib/Parse/ParseStmt.cpp
318–327

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.

337–339

I don't feel super strongly about it, so this can wait until a follow-up if we want to do anything about it.

clang/lib/Sema/ParsedAttr.cpp
224

... 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.

clang/lib/Sema/SemaDeclAttr.cpp
8371–8372

Fine by me.

8381

isa is variadic.

8385

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.

clang/lib/Sema/SemaType.cpp
1814

SGTM!

mboehme updated this revision to Diff 433052.May 31 2022, 4:38 AM
mboehme marked 3 inline comments as done.

Changes in response to review comments

mboehme retitled this revision from [clang] [WIP] Reject non-declaration C++11 attributes on declarations to [clang] Reject non-declaration C++11 attributes on declarations.May 31 2022, 4:40 AM
mboehme updated this revision to Diff 433055.May 31 2022, 5:08 AM
mboehme marked 8 inline comments as done.

Changes in response to review comments.

mboehme marked 8 inline comments as done.May 31 2022, 1:04 PM

I notice that https://reviews.llvm.org/D111548, which this change is based on, has some failing flang tests in the CI. I'm pretty sure these are unrelated and stem from the base revision that I happen to be based off. Before rebasing, however, I wanted to make sure all of the big comments have been resolved so that I don't introduce too much churn into the diffs.

clang/include/clang/Sema/DeclSpec.h
1853–1858

Sorry, I don't know what I was thinking when I wrote DeclSpec. That should have read "Attributes attached to the declarator", and I've changed it accordingly.

I hope that with this change, this makes sense now?

1924–1926

Just making sure I can disregard this comment? Please respond if there's something you'd like me to address here.

clang/include/clang/Sema/ParsedAttr.h
664

I've decided to add LegacyBehavior at the end -- WDYT?

clang/lib/Parse/ParseDecl.cpp
1848

Thanks for pointing this out!

I've removed the ParseAttributes(DeclSpecAttrs) call and have added tests for the behavior of free-standing decl-specifier-seqs in test/Parser/attributes.c (for empty __attribute__(())) and test/Sema/attr-declspec-ignored.c (to make sure that we produce the correct warnings on block declarations).

clang/lib/Parse/ParseDeclCXX.cpp
2691–2696

Yes, done. Nice!

clang/lib/Parse/ParseExprCXX.cpp
2745–2747

Good point -- done (and changed Declarator to hold a const ParsedAttributesView&).

clang/lib/Parse/ParseStmt.cpp
16

Thanks, done.

243–248

Just confirming: You're not asking me to do anything here, correct?

318–327

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?

clang/lib/Sema/ParsedAttr.cpp
224

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?

240–243

Many of these were previously not permitted in type attribute position, despite being declared in Attr.td as TypeAttrs. 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:

  1. 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().
  2. noderef. It turns out that this attribute currently does not work at all in the [[]] spelling; I've written up a bug 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.

clang/lib/Sema/SemaDeclAttr.cpp
8325–8326

My previous comment was based merely on the fact that I sometimes observed the attribute on the DeclSpec, but I did not dig any further at the time. I should have because I would have discovered the actual reason, namely that isCXX11Attribute() erroneously classifies the _Alignas spelling of the attribute as a C++11 attribute. See the changed comment above and the comment in isAlignasAttribute() for details.

I'm not sure how isAlignasAttribute() should best be fixed, so I would propose leaving the current logic in place for the time being as a workaround.

8381

Nice. Done!

8385

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.

clang/test/Parser/MicrosoftExtensions.cpp
65

The error with snippet and caret looks like this:

MicrosoftExtensions.cpp:66:50: error: expected member name or ';' after declaration specifiers
  [uuid("000000A0-0000-0000-C000-00000000004A")] using base::a; // expected-error {{expected member name}}
                                                 ^

I would classify that as "not great, not terrible". I think it's defensible given that a) I think it's an error few people are likely to make, so few people will see the error message, and b) MSVC's error message isn't great either:

<source>(2): error C2059: syntax error: 'using'
<source>(2): error C2238: unexpected token(s) preceding ';'

(see https://godbolt.org/z/jrx6Y1Y83)

If we wanted a better error message, we'd have to do the MaybeParseMicrosoftAttributes() somewhere above ParseDeclCXX.cpp:2691, then add a ProhibitAttributes() inside the if (Tok.is(tok::kw_using)). I'm not sure if this is ultimately worth it though?

clang/test/SemaCXX/address-space-placement.cpp
4–6

Good point. I've added those tests.

rsmith added inline comments.Jun 8 2022, 2:11 PM
clang/lib/Parse/ParseStmt.cpp
243–248

Yes, no action requested. Thanks for talking through this option with me! :)

clang/lib/Sema/ParsedAttr.cpp
228

I don't think this one really slides today, in the syntactic sense, prior to this patch (and I guess the same is true for most of these). Given:

[[clang::address_space(5)]] int x;
int [[clang::address_space(5)]] y;

... we accept x and reject y, making it look like this is simply a declaration attribute. Hm, but we accept:

int *[[clang::address_space(5)]] *z;

... so it really *does* seem to behave like a type attribute sometimes?

OK, so this patch is doing two things:

  1. It's permitting the use of these attributes as type attributes, where they were not consistently permitted previously.
  2. It's deprecating / warning on uses of them as declaration attributes.

That means there's no way to write code that is compatible with both old and new Clang and produces no warnings, but it's easy enough to turn the warning flag off I suppose. The new behavior does seem much better than the old.

240–243

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 TypedefNameDecls) and a type attribute (we find it on AttributedTypes)

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?

241

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?

mboehme updated this revision to Diff 435525.Jun 9 2022, 6:22 AM
mboehme marked 8 inline comments as done.
  • Changes in response to review comments
  • Rebase to newer base revision
mboehme marked an inline comment as done.Jun 9 2022, 6:32 AM
mboehme added inline comments.
clang/lib/Sema/ParsedAttr.cpp
228

I don't think this one really slides today, in the syntactic sense, prior to this patch (and I guess the same is true for most of these). Given:

[[clang::address_space(5)]] int x;
int [[clang::address_space(5)]] y;

... we accept x and reject y, making it look like this is simply a declaration attribute.

Yes, and we get the same behavior for all C++11 attributes.

This is because, today, Clang flat-out rejects _all_ C++11 attributes that are placed after a decl-specifier-seq:

From ParseDeclarationSpecifiers():

// Reject C++11 attributes that appertain to decl specifiers as
// we don't support any C++11 attributes that appertain to decl
// specifiers. This also conforms to what g++ 4.8 is doing.
ProhibitCXX11Attributes(attrs, diag::err_attribute_not_type_attr);

https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L3182

So really what the "slides" terminology means is "is treated as appertaining to the decl-specifier-seq instead of the declaration".

Hm, but we accept:

int *[[clang::address_space(5)]] *z;

... so it really *does* seem to behave like a type attribute sometimes?

Yes, because Clang does allow C++11 attributes (including address_space in particular) on declarator chunks today -- it just doesn't allow them on the decl-specifier-seq. As an aside, this makes the error message we get when putting the attribute on the decl-specifier-seq today ("'address_space' attribute cannot be applied to types") pretty misleading.

OK, so this patch is doing two things:

  1. It's permitting the use of these attributes as type attributes, where they were not consistently permitted previously.
  2. It's deprecating / warning on uses of them as declaration attributes.

Actually, 1) happens in https://reviews.llvm.org/D111548, which this change is based on.

I did things this way because

  • I want to introduce annotate_type first, so that this change can use it in tests as an example of a type attribute that doesn't "slide"
  • I wanted tests in https://reviews.llvm.org/D111548 to be able to use annotate_type in all places that it's intended for, including on the decl-specifier-seq, so I needed to open up that usage. (Actually, when I first authored https://reviews.llvm.org/D111548, I didn't even realize that this followup change would be coming.)

Is this confusing? I could instead do the following:

Do you think that would be preferable?

That means there's no way to write code that is compatible with both old and new Clang and produces no warnings, but it's easy enough to turn the warning flag off I suppose.

That's true -- and I think it's the best we can do. The behavior that Clang exhibits today is of course set in stone (error out if the attribute is placed on the decl-specifier-seq), and at the same time it seems obvious that we want to nudge people away from putting the attribute on the declaration, as it's really a type attribute. (Just repeating your analysis here essentially; I think we agree, and there's nothing to do here.)

I do think this makes an argument that the new diagnostic we're introducing should be a warning initially, not an error. Otherwise, there would be no way to use the C++11 spelling in a way that compiles (albeit maybe with warnings) both before and after this patch.

The new behavior does seem much better than the old.

240–243

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 TypedefNameDecls) and a type attribute (we find it on AttributedTypes)

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.

241

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 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.
mboehme updated this revision to Diff 435823.Jun 10 2022, 12:59 AM
mboehme marked an inline comment as done.Jun 10 2022, 12:59 AM

Fix typos in comments.

@aaron.ballman Any further comments from you on this change?

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

@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.

clang/include/clang/Basic/AttributeCommonInfo.h
153

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.

clang/include/clang/Sema/DeclSpec.h
1912

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?

1924–1926

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?

clang/include/clang/Sema/ParsedAttr.h
664

Works for me

1120

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.

clang/lib/Parse/ParseStmt.cpp
230

Coding style nit.

231–238

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).

249–253

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?

318–327

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.

clang/lib/Sema/ParsedAttr.cpp
224

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).

240–243

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.

clang/lib/Sema/SemaDeclAttr.cpp
8381

This shouldn't need to be qualified and the extra parens aren't needed.

8385

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.

rsmith accepted this revision.Jun 13 2022, 1:16 PM

This is a bold direction but I like it a lot. Over to @aaron.ballman for final approval.

clang/lib/Sema/ParsedAttr.cpp
224

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.

228

Thanks for the explanation. If we're landing both patches more or less together anyway, I'm not too concerned by how we split the changes between them. No action required here, I think.

241

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.

This revision is now accepted and ready to land.Jun 13 2022, 1:16 PM
mboehme updated this revision to Diff 436734.Jun 14 2022, 4:07 AM

Generally allow type attributes to be placed on the decl-specifier-seq (with
some exceptions for specific attributes that were already present in this
change).

Previously, https://reviews.llvm.org/D111548, which this change is based on,
had already allowed all type attributes to be placed on the decl-specifier-seq,
but @aaron.ballman's comments on that patch made me realize that the change is
better made here.

aaron.ballman accepted this revision.Jun 14 2022, 5:05 AM

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

clang/lib/Sema/ParsedAttr.cpp
224

Okay, that's convincing, thanks!

241

+1 to this :-)

mboehme updated this revision to Diff 436755.Jun 14 2022, 5:55 AM

Changes in response to review comments.

mboehme marked 11 inline comments as done.Jun 14 2022, 6:15 AM

Only thing left for me are the nits I pointed out in the last review, otherwise I think this is all set.

Thanks for the quick turnaround!

Giving my LG because I don't think we need another round of review for those nits unless you want it

Just two things I'd appreciate a quick confirmation on:

  • My question on isAlignasAttribute()
  • Feedback on whether you think my refactoring of the complex boolean expression makes sense
clang/include/clang/Basic/AttributeCommonInfo.h
153

Thanks for pointing this out! Is there anything specific I should add to the code comment, or is your comment just for general awareness?

clang/include/clang/Sema/DeclSpec.h
1912

No problem -- done!

1924–1926

Good point -- done.

clang/include/clang/Sema/ParsedAttr.h
1120

Good point. Done, with a slightly different name -- WDYT?

clang/lib/Parse/ParseStmt.cpp
230

Done.

231–238

Yes, this is confusing. I've factored out some variables that I hope make this more readable.

By the way, it might look as if GNUAttributeLoc.isValid() implies HaveAttributes and that the latter is therefore redundant, but this isn’t actually true if we failed to parse the GNU attribute. Removing the HaveAttributes makes some tests fail, e.g. line 78 of clang/test/Parser/stmt-attributes.c.

249–253

Yes, we can rely on this being the case. This function is called from only one place where both CXX11Attrs and GNUAttrs can potentially contain attributes, namely ParseStatementOrDeclaration() (line 114 in this file). There, the CXX11Attrs are parsed before the GNUAttrs. I’ve added an assertion, however, since this guarantee is important to the correctness of the code here.

318–327

You've convinced me -- done!

clang/lib/Sema/ParsedAttr.cpp
228

As it happens, I did end up making a change here after all:

  • https://reviews.llvm.org/D111548 only allows annotate_type but no other attributes on the decl-specifier-seq
  • This patch allows type attributes more generally on the decl-specifier-seq

See also the discussion on https://reviews.llvm.org/D111548 for more background on why I moved the code around like this.

Given the previous discussion, I'm assuming you're OK with this change. (The final state of the code once both patches have landed will be the same as it was before -- this is just changing which patch does what.)

clang/lib/Sema/SemaDeclAttr.cpp
8381

Done. (Sorry for not getting this right the first time.)

8385

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.

That makes sense -- and I've only come to appreciate while working on this patch how much variation and unpredictability there is in type attributes. Thanks for the discussion and clarifications.

aaron.ballman accepted this revision.Jun 14 2022, 7:36 AM

LGTM!

clang/include/clang/Basic/AttributeCommonInfo.h
153

Nope, just spreading the information around to more people's brains -- nothing needs to be changed here.

clang/include/clang/Sema/ParsedAttr.h
1120

Works great for me!

clang/lib/Parse/ParseStmt.cpp
230

I don't insist, but I think it's a tiny bit easier to read this way.

231–238

Thanks, I think this is more readable this way!

249–253

Thank you for the confirmation and the added assertion!

318–327
mboehme updated this revision to Diff 437039.Jun 14 2022, 10:52 PM
mboehme marked 8 inline comments as done.

Small code tweaks

mboehme marked 2 inline comments as done.Jun 14 2022, 10:54 PM

Thank you @aaron.ballman and @rsmith for the careful and productive reviews!

clang/lib/Parse/ParseStmt.cpp
230

Done!

318–327

Done!

This revision was landed with ongoing or failed builds.Jun 15 2022, 2:58 AM
This revision was automatically updated to reflect the committed changes.
mboehme marked 2 inline comments as done.
nikic added a subscriber: nikic.Jun 15 2022, 8:42 AM

FYI this change had a measurable effect on compile-time (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb&stat=instructions), about 0.5% regression for O0 builds. Not sure if that's expected.

FYI this change had a measurable effect on compile-time (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb&stat=instructions), about 0.5% regression for O0 builds. Not sure if that's expected.

Thanks for the heads-up.

Without more extensive investigation, it's hard to say. I wasn't necessarily expecting a slowdown, but I can come up with various hypotheses for what might be causing it.

I've seen the instructions on the About page on how to reproduce results and will give that a go.

The first thing to analyze would be whether the slowdown occurs only on code that uses attributes (heavily) or also on code that doesn't use attributes.

There isn't any single substantial change in the code that would cause an obvious slowdown. I could speculate on some potential causes, but it seems better to measure instead. Is there any particular tooling that you would recommend for comparing profiler runs from two different compiler versions (one with my patch, one without)?

I'll post an update here when I find out more.

dyung added a subscriber: dyung.Jun 16 2022, 7:01 PM

@mboehme, one of our internal tests started to fail to compile after this change due to the compiler no longer accepting what I think should be a valid attribute declaration. I have filed issue #56077 for the problem, can you take a look?

@mboehme, one of our internal tests started to fail to compile after this change due to the compiler no longer accepting what I think should be a valid attribute declaration. I have filed issue #56077 for the problem, can you take a look?

Attributes are not allowed to occur in this position -- I'll discuss in more detail on #56077.

See also the response I'm making on a code comment, where a reviewer pointed out to me that the C++ standard does not allow [[]] attributes before extern "C".

clang/lib/Parse/Parser.cpp
1165

Update: @dyung is seeing errors on their codebase because of this -- see also the comment they added to this patch.

It's not unexpected that people would have this incorrect usage in their codebases because we used to allow it. At the same time, it's not hard to fix, and I would generally expect this kind of error to occur in first-party code (that is easily fixed) rather than third-party libraries, because the latter usually compile on GCC, and GCC disallows this usage.

Still, I wanted to discuss whether you think we need to reinstate support for this (incorrect) usage temporarily, with a deprecation warning rather than an error?

aaron.ballman added inline comments.Jun 17 2022, 5:01 AM
clang/lib/Parse/Parser.cpp
1165

Still, I wanted to discuss whether you think we need to reinstate support for this (incorrect) usage temporarily, with a deprecation warning rather than an error?

I think it depends heavily on what's been impacted. If it's application code, the error is fine because it's pretty trivial to fix. If it's system header or popular 3rd party library code, then a warning might make more sense.

It's worth noting that what we accepted didn't have any semantic impact anyway. We would accept the attribute, but not actually add it into the AST: https://godbolt.org/z/q7n1794Tx. So I'm not certain there's any harm aside from the annoyance of getting an error where you didn't previously get one before.

mboehme added a comment.EditedJun 17 2022, 1:24 PM

FYI this change had a measurable effect on compile-time (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb&stat=instructions), about 0.5% regression for O0 builds. Not sure if that's expected.

I've found the reason for this slowdown, and a fix.

In a number of places, this patch adds additional local variables of type ParsedAttributes, typically because we need to keep track of declaration and decl-specifier-seq attributes in two separate ParsedAttribute lists where previously we were putting them in the same list. Note that in the vast majority of cases, these ParsedAttributes lists are empty.

I would have assumed that creating an empty ParsedAttributes, potentially iterating over it, then destroying it, is cheap. However, this is not true for ParsedAttributes because it uses a TinyPtrVector as the underlying container. The same is true for the AttributePool that ParsedAttributes contains.

TinyPtrVector is amazingly memory-frugal in that it consumes only a single pointer worth of memory if the vector contains zero or one elements. However, this comes at a cost: begin() and end() on this container are relatively expensive. As a result, iterating over an empty TinyPtrVector is relatively expensive.

The destructor of ParsedAttributes iterates over its AttributePool. As a result, merely creating an empty ParsedAttributes, then destroying it is a relatively slow operation. My patch does this on the order of one additional time per declaration, and this is enough to cause the observed slowdown.

The fix for this is simply to replace the TinyPtrVector in ParsedAttributes and AttributePool with a SmallVector. The extreme memory frugality of TinyPtrVector is not actually necessary in ParsedAttributes and AttributePool: These objects are not part of the AST; they are only allocated on the heap, and only a small number of them are ever in existence at any given time. Spending a few bytes more on a SmallVector has a negligible effect on memory usage, but makes iterating over the vector much faster.

In fact, making this change not only fixes the slowdown observed on this patch but actually makes compiles run slightly faster than they did before this patch.

I'll be submitting a patch with the fix described above but wanted to get this information out ahead of time.

Fix for compile time regression submitted for review at https://reviews.llvm.org/D128097

https://reviews.llvm.org/D128097 is now submitted. Unfortunately, it didn't fix the compile-time regression entirely (see detailed comments there).

I'll do some more investigation to see where the rest of the slowdown is coming from.

miyuki added a subscriber: miyuki.EditedJun 23 2022, 8:09 AM

Hi Martin,

I think this patch caused a regression in diagnostics. Clang used to warn about the following code:

struct X {
    [[deprecated]] struct Y {};
};
$ clang -std=c++17 -Wall test.cpp
<source>:2:7: warning: attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes]
    [[deprecated]] struct Y {};
      ^

And now it doesn't. Could you please have a look into it?

Hi Martin,

I think this patch caused a regression in diagnostics. Clang used to warn about the following code:

struct X {
    [[deprecated]] struct Y {};
};
$ clang -std=c++17 -Wall test.cpp
<source>:2:7: warning: attribute 'deprecated' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes]
    [[deprecated]] struct Y {};
      ^

And now it doesn't. Could you please have a look into it?

Good catch, that does appear to be a regression:

http://eel.is/c++draft/class#nt:member-declaration
http://eel.is/c++draft/class#mem.general-14

Hi Martin,

I think this patch caused a regression in diagnostics. Clang used to warn about the following code:

[snip]

And now it doesn't. Could you please have a look into it?

Thanks for alerting me to this! I'll investigate and will submit a fix.