[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 11 inline comments as done.
mboehme added a comment.

In D126061#3581350 <https://reviews.llvm.org/D126061#3581350>, @aaron.ballman wrote:

> 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



================
Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
     // FIXME: Use a better mechanism to determine this.
+    // We use this in `isCXX11Attribute` below, so it _should_ only return
+    // true for the `alignas` spelling, but it currently also returns true
----------------
aaron.ballman wrote:
> 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.
Thanks for pointing this out! Is there anything specific I should add to the code comment, or is your comment just for general awareness?


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1916
+  /// declaration, i.e. `x` and `y` in this case.
+  Declarator(const DeclSpec &ds, const ParsedAttributesView &DeclarationAttrs,
+             DeclaratorContext C)
----------------
aaron.ballman wrote:
> 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?
No problem -- done!


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1928-1930
+    assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) {
+      return AL.isStandardAttributeSyntax();
+    }));
----------------
aaron.ballman wrote:
> 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?
Good point -- done.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and `Second`.
+void ConcatenateAttributes(ParsedAttributes &First, ParsedAttributes &Second,
+                           ParsedAttributes &Result);
----------------
aaron.ballman wrote:
> 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.
Good point. Done, with a slightly different name -- WDYT?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+    auto isStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); };
     if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
----------------
aaron.ballman wrote:
> Coding style nit.
Done.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
     if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
          (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
              ParsedStmtContext()) &&
         ((GNUAttributeLoc.isValid() &&
-          !(!Attrs.empty() &&
-            llvm::all_of(
-                Attrs, [](ParsedAttr &Attr) { return Attr.isStmtAttr(); }))) ||
+          !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+            llvm::all_of(CXX11Attrs, isStmtAttr) &&
+            llvm::all_of(GNUAttrs, isStmtAttr))) ||
----------------
aaron.ballman wrote:
> 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).
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.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+      if (CXX11Attrs.Range.getBegin().isValid())
+        DeclStart = CXX11Attrs.Range.getBegin();
+      else if (GNUAttrs.Range.getBegin().isValid())
+        DeclStart = GNUAttrs.Range.getBegin();
----------------
aaron.ballman wrote:
> 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?
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.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
   case tok::kw_asm: {
-    ProhibitAttributes(Attrs);
+    ProhibitAttributes(CXX11Attrs);
+    ProhibitAttributes(GNUAttrs);
     bool msAsm = false;
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > It seems like we might have a pre-existing bug here for [[]] attributes? http://eel.is/c++draft/dcl.dcl#nt:asm-declaration
> > > > Yes, we do!
> > > > 
> > > > Fixed, and tests added in test/Parser/asm.c and test/Parser/asm.cpp.
> > > I think we should prohibit this for all `[[]]` attributes. C doesn't have an asm statement as part of the standard, but it lists it as a common extension in Annex J and uses the same syntax as the C++ feature. Might as well keep it consistent between the languages.
> > 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?
> > 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.
You've convinced me -- done!


================
Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+  switch (getParsedKind()) {
+  case AT_AddressSpace:
+  case AT_OpenCLPrivateAddressSpace:
----------------
rsmith wrote:
> mboehme wrote:
> > rsmith wrote:
> > > 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.
> > > 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:
> > 
> >   - Move the change that allows C++11 attributes on the decl-specifier-seq from https://reviews.llvm.org/D111548 to here.
> >   - Move tests that use `annotate_type` on the decl-specifier-seq from https://reviews.llvm.org/D111548 to here.
> > 
> > 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.
> > 
> > 
> 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.
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.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8387
+      if (AL.slidesFromDeclToDeclSpecLegacyBehavior() &&
+          (clang::isa<DeclaratorDecl, TypeAliasDecl>(D))) {
+        // Suggest moving the attribute to the type instead, but only for our
----------------
aaron.ballman wrote:
> This shouldn't need to be qualified and the extra parens aren't needed.
Done. (Sorry for not getting this right the first time.)


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385
+        // portability.
+        if (AL.isClangScope()) {
+          S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl)
----------------
aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > 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.
> > > 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.
> > 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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126061/new/

https://reviews.llvm.org/D126061