Page MenuHomePhabricator

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

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
1915

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?

1927–1929

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

1126

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
229

Coding style nit.

233–237

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

248–256

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?

321–326

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
8387

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

8391

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.Mon, Jun 13, 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.Mon, Jun 13, 1:16 PM
mboehme updated this revision to Diff 436734.Tue, Jun 14, 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.Tue, Jun 14, 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.Tue, Jun 14, 5:55 AM

Changes in response to review comments.

mboehme marked 11 inline comments as done.Tue, Jun 14, 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
1915

No problem -- done!

1927–1929

Good point -- done.

clang/include/clang/Sema/ParsedAttr.h
1126

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

clang/lib/Parse/ParseStmt.cpp
229

Done.

233–237

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.

248–256

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.

321–326

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
8387

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

8391

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.Tue, Jun 14, 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
1126

Works great for me!

clang/lib/Parse/ParseStmt.cpp
229

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

233–237

Thanks, I think this is more readable this way!

248–256

Thank you for the confirmation and the added assertion!

321–326
mboehme updated this revision to Diff 437039.Tue, Jun 14, 10:52 PM
mboehme marked 8 inline comments as done.

Small code tweaks

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

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

clang/lib/Parse/ParseStmt.cpp
229

Done!

321–326

Done!

This revision was landed with ongoing or failed builds.Wed, Jun 15, 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.Wed, Jun 15, 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.Thu, Jun 16, 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.Fri, Jun 17, 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.EditedFri, Jun 17, 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.EditedThu, Jun 23, 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.