rsandifo-arm added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2000 "overridden virtual function is here">; +def err_conflicting_overriding_attributes : Error< + "virtual function %0 has different attributes " ---------------- rsandifo-arm wrote: > aaron.ballman wrote: > > aaron.ballman wrote: > > > rsandifo-arm wrote: > > > > sdesmalen wrote: > > > > > aaron.ballman wrote: > > > > > > sdesmalen wrote: > > > > > > > aaron.ballman wrote: > > > > > > > > This error and the one below make me wonder whether an > > > > > > > > attribute spelling is the appropriate way to surface this > > > > > > > > functionality as opposed to a keyword. Attributes don't > > > > > > > > typically don't cause errors in these situations, but because > > > > > > > > these attributes are strongly coupled to their type system > > > > > > > > effects I can see why you want these to be errors. > > > > > > > > This error and the one below make me wonder whether an > > > > > > > > attribute spelling is the appropriate way to surface this > > > > > > > > functionality as opposed to a keyword. > > > > > > > I'm not sure I understand what you mean, do you have an example? > > > > > > `override` and `final` could have been surfaced via attributes, and > > > > > > in Clang we semantically express them as such (see `Final` and > > > > > > `Override` in Attr.td), but instead they are surfaced to the user > > > > > > as keywords in the language standard. You're not under the same > > > > > > constraints as the standard (you're making a vendor-specific > > > > > > attribute). We're not super consistent with whether we use the same > > > > > > approach as the standard (we have some type attributes that are > > > > > > spelled as attributes like `vector_size` and others that are > > > > > > spelled via keywords like `_Nullable`. > > > > > > > > > > > > So you could spell your type attributes the way you have been with > > > > > > `__attribute__`, or you could come up with keywords for them (so > > > > > > instead of using `GNU<"whatever">` for the attribute, you could use > > > > > > `Keyword<_Whatever>` or `Keyword<__whatever>` (you'd also need to > > > > > > add them as recognized keyword tokens, parse them as appropriate, > > > > > > etc). > > > > > > > > > > > > Note: I don't insist on you using a keyword for these, but I am > > > > > > wondering if that approach was considered or not given how > > > > > > non-portable the attributes are (if an implementation ignores this > > > > > > attribute, it sounds like the program semantics are unlikely to be > > > > > > correct). > > > > > @rsandifo-arm can you shed some light on whether using a keyword > > > > > instead of an `attribute` was considered? > > > > Thanks @aaron.ballman for the reviews. > > > > > > > > Yeah, we did consider using keywords instead. The main reason for > > > > sticking with GNU attributes is that they were specifically designed as > > > > an extensible way of attaching information or requirements to the > > > > source code, and they provide well-settled semantics. It seemed better > > > > to reuse an existing mechanism rather than invent something new. > > > > > > > > Also, as C++ evolves, the semantics of GNU attributes evolve to match. > > > > If we surface the SME features as GNU attributes, we inherit this > > > > development in the underlying semantics, without having to maintain our > > > > own evolving specification for where the keywords can be placed. For > > > > example, when lambdas were introduced, GNU attributes were extended to > > > > support lambdas. Something similar could happen in future. > > > > > > > > We could have defined the semantics of the keywords to be that they > > > > behave exactly like GNU attributes. However: > > > > > > > > # If we do that, the advantage of using a keyword is less obvious. (I > > > > can see the argument that the syntax looks nicer though.) > > > > # Like you say in the review, the semantics of GNU attributes carry > > > > some historical baggage. If would be difficult to justify keeping that > > > > historical baggage for something that is ostensibly new and not a GNU > > > > attribute as such. > > > > > > > > A minor, but still nontrivial, reason is that there is less appetite in > > > > the GCC community for supporting target-specific extensions to the core > > > > language. Adding a target-specific keyword is likely to be rejected. > > > > It would be acceptable to make the “keyword” be a predefined macro that > > > > expands to a GNU attribute under the hood, but I don't think that would > > > > address the core issue. > > > > > > > > I can definitely understand the unease about using attributes for > > > > things that affect semantics. Like you say, that's going against a > > > > core principle of the standard-defined attributes. But I think in a > > > > way, it's unfortunate that GNU-style attributes and standard-defined > > > > attributes are both called “attributes”, because I don't think there's > > > > a prohibition (even in theory) against GNU attributes affecting > > > > semantics. I think GNU attributes are designed to be more general than > > > > that, probably through historical accretion rather than an up-front > > > > choice. > > > > > > > > Like you say, `vector_size` is one example of something that > > > > significantly affect semantics. But there are quite a few others too. > > > > For example: > > > > * GNU targets traditionally provide access to naked functions and > > > > interrupt handlers through attributes. > > > > * Different calling conventions also use attributes. > > > > * The `target` attribute switches between ISAs in ways that do affect > > > > semantics. > > > > * `always_inline` functions must be inlined for correctness. > > > > * `weak` and `visibility` in effect alter the ODR. > > > > * In GCC, functions that return twice (like `setjmp`) must be declared > > > > `returns_twice`. > > > > > > > > It's unfortunate that using SME attributes will only trigger a warning > > > > in older compilers. The warning is enabled by default though. And in > > > > practice, I think most SME code would rely on other constructs that > > > > older compilers wouldn't provide, such as intrinsics. > > > > Yeah, we did consider using keywords instead. The main reason for > > > > sticking with GNU attributes is that they were specifically designed as > > > > an extensible way of attaching information or requirements to the > > > > source code, and they provide well-settled semantics. It seemed better > > > > to reuse an existing mechanism rather than invent something new. > > > > > > If this is extra information the compiler is free to ignore without > > > impacting program correctness, then I think an attribute is fine. But if > > > the user's code will silently break if the attribute is ignored (e.g., an > > > attribute ignored warning happens but the program successfully translates > > > and does bad things at runtime), in some ways use of an attribute is far > > > less ideal than use of a keyword-based attribute specifically because of > > > the syntax error the user would get. Some users really like the "unknown > > > attribute ignored" warning (like me!) and ensure it stays enabled and > > > others users really don't like that warning and disable it. So it's > > > dangerous to assume there's anything there worth relying on if the > > > attributes have security implications at runtime. > > > > > > > Also, as C++ evolves, the semantics of GNU attributes evolve to match. > > > > > > Errr does it? > > > https://gcc.gnu.org/onlinedocs/gcc/extensions-to-the-c-language-family/attribute-syntax.html#attribute-syntax > > > "There are some problems with the semantics of attributes in C++. For > > > example, there are no manglings for attributes, although they may affect > > > code generation, so problems may arise when attributed types are used in > > > conjunction with templates or overloading. Similarly, typeid does not > > > distinguish between types with different attributes. Support for > > > attributes in C++ may be restricted in future to attributes on > > > declarations only, but not on nested declarators." doesn't really seem to > > > support that position. > > > > > > > A minor, but still nontrivial, reason is that there is less appetite in > > > > the GCC community for supporting target-specific extensions to the core > > > > language. > > > > > > That's unfortunate given that `_Keywords` are reserved to the > > > implementation specifically for this sort of thing. `_Nullable` is a good > > > example of a successful attribute keyword. Attributes are a > > > target-specific extension to the core language regardless of what syntax > > > they use, so to me the driving question is more "what user experience do > > > you want?" and less "is this an extension to the core language?" > > Re-pinging this part of the thread -- the more I review this, the more it > > sounds like ignoring these attributes will cause a significant likelihood > > of the user's program silently misbehaving. Is that accurate? If so, I > > would reconsider whether using a keyword gives a more appropriate user > > experience for portability. With an attribute, the user has a very real > > chance of their code continuing to compile but not work. With a keyword, > > the user will get errors when porting to a compiler that doesn't support > > the keyword and are more protected from miscompiles. We can still reuse the > > vast majority of the implementation work already done here, so it should > > not be a major change to the implementation (a bit of extra parsing work > > for the keywords is really the only thing missing). > I accept your points about the dangers of ignoring unsupported attributes. > But this isn't an SME-specific issue. Ignoring the other GNU attributes I > mentioned would also silently generate wrong code. I think this is a very > strong argument against suppressing warnings about unrecognised GNU > attributes by default, even if warnings about unrecognised standard > attributes are eventually suppressed by default. (Perhaps they should even > be controllable separately?) Like I say, there doesn't seem to have been a > principle that GNU attributes provide information that the compiler is free > to ignore. > > If we did add keywords, would you be OK with giving them exactly the same > semantics as GNU attributes? In other words, would it be acceptable to treat > the keywords as essentially a one-for-one parsing replacement for GNU > attributes? Or would you want the keyword to follow the same distinction > between type properties and object properties as the standard attributes do? > Or would you want the keyword to follow the same distinction between type > properties and object properties as the standard attributes do? That was a vague question, sorry. What I meant is whether the correct position of the keyword should vary based on whether it describes a property of the function type vs. a property of the function definition, or whether we can maintain the laxity in the GNU attribute positioning. I think the laxity in the GNU attribute positioning is helpful to users, and in this case wouldn't introduce any ambiguity. From discussions I've had with people who will write/are writing SME code, the distinction between `arm_locally_streamng` being a property of the definition and `arm_streaming` being a property of the type seems contrived to them, especially when the usage of SME is private to a single translation unit. Treating the keyword as an error-generating GNU attribute stand-in would also allow implementations to make the keyword as a preprocessor macro, if they need to. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127762/new/ https://reviews.llvm.org/D127762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
