aaron.ballman added a comment. In D126061#3577054 <https://reviews.llvm.org/D126061#3577054>, @mboehme wrote:
> @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. ================ 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 ---------------- 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. ================ 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) ---------------- 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? ================ Comment at: clang/include/clang/Sema/DeclSpec.h:1928-1930 + assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) { + return AL.isStandardAttributeSyntax(); + })); ---------------- 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? ================ 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); ---------------- 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. ================ Comment at: clang/include/clang/Sema/ParsedAttr.h:664 + /// This may only be called if isStandardAttributeSyntax() returns true. + bool slidesFromDeclToDeclSpec() const; + ---------------- mboehme wrote: > aaron.ballman wrote: > > 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. > I've decided to add `LegacyBehavior` at the end -- WDYT? Works for me ================ Comment at: clang/lib/Parse/ParseStmt.cpp:229 default: { + auto isStmtAttr = [](ParsedAttr &Attr) { return Attr.isStmtAttr(); }; if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt || ---------------- Coding style nit. ================ 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))) || ---------------- 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). ================ 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(); ---------------- 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? ================ Comment at: clang/lib/Parse/ParseStmt.cpp:317-318 case tok::kw_asm: { - ProhibitAttributes(Attrs); + ProhibitAttributes(CXX11Attrs); + ProhibitAttributes(GNUAttrs); bool msAsm = false; ---------------- 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. ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:240-243 + case AT_Regparm: + case AT_NoDeref: + case AT_ObjCGC: + case AT_VectorSize: ---------------- mboehme wrote: > rsmith wrote: > > mboehme wrote: > > > rsmith wrote: > > > > Many of these were previously not permitted in type attribute position, > > > > despite being declared in Attr.td as `TypeAttr`s. 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. > > > > Many of these were previously not permitted in type attribute position, > > > > despite being declared in Attr.td as `TypeAttr`s. 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: > > > > > > # `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()`. > > > > > > # `noderef`. It turns out that this attribute currently does not work > > > at all in the `[[]]` spelling; I've written up a > > > [bug](https://github.com/llvm/llvm-project/issues/55790) 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. > > > 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 `TypedefNameDecl`s) and a > > type attribute (we find it on `AttributedType`s) > > > > 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? > > 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 `TypedefNameDecl`s) and a > > type attribute (we find it on `AttributedType`s) > > 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. > 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. ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:224 + // property is consciously not defined as a flag in Attr.td because we don't + // want new attributes to specify it. + switch (getParsedKind()) { ---------------- mboehme wrote: > aaron.ballman wrote: > > mboehme wrote: > > > aaron.ballman wrote: > > > > Thoughts on the additional comment? And should we start to issue that > > > > deprecation warning now (perhaps as a separate follow-up commit)? > > > > 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. > > > ... 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`. > > > 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? 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). ================ 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 ---------------- This shouldn't need to be qualified and the extra parens aren't needed. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8385 + // portability. + if (AL.isClangScope()) { + S.Diag(AL.getLoc(), diag::warn_type_attribute_deprecated_on_decl) ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126061/new/ https://reviews.llvm.org/D126061 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
