RFR: 8302124: HotSpot Style Guide should permit noreturn attribute

[Julian Waters]

On Mon, 13 Feb 2023 04:57:48 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> As for the function declaration, it's pretty much the entire function, keywords and all, as the C++ Standard always has the attribute specifier sequence appear before everything else (alignas is also an attribute specifier, to that extent), and never between keywords. I'm in favour of adding an extra line also forbidding having the attributes placed behind the function's parameter list brackets though, since HotSpot currently has a ton of attributes that are listed behind the whole function
>> 
>> I think Kim has stated before that compiler specific attributes don't give much benefit and isn't really interested in having them, that said it would be nice to have them in the future with perhaps the compiler's namespace required in the attribute (msvc::noinline in C++20 guarantees no inlining of a function as compared to our current __declspec(noinline) for instance).
>
> Standard attributes are syntactically permitted in a plethora of places.  But
> specific attributes are only allowed in places related to their semantics.
> For function declarations (including definitions), this is either first,
> before anything else related to the declaration, or after the name of the
> function but before the parameter list.
> 
> What I've proposed is that HotSpot code prefers putting function attributes
> first, rather than immediately after the function name (so before the
> parameter list).
> 
> Standard attributes can also appear after the parameter list, but those apply
> to the function *type* rather than the function.  That differs from gcc
> `__attribute__`.
> 
> (Standard attributes can also appear after the return type but before the
> function name.  Those attributes apply to the return type.  Hoping we don't
> need any of those, other than perhaps for alignment.)
> 
> I've no idea how standard attributes and gcc `__attribute__` or msvc `__declspec`
> interact syntactically.  My hope would be that they can interleave, but I have
> no idea whether that's true or not.  I guess some research is needed, though I
> think it doesn't matter for the current set.  (I doubt anyone is going to
> declare a function both noreturn and always-inline, for example.)
> 
> My objection to the earlier proposal to (for example) change ATTRIBUTE_PRINTF
> to use `[[gnu::format(printf...)]]` instead of the `__attribute__` form was
> that it required moving a lot of the macro uses from the end of a declaration
> to its front (because of the above-mentioned difference), so lots of code
> changes for not much gain.
> 
> Unrecognized attributes cause problems.  C++17 changes that, requiring that
> unrecognized attributes be ignored.  That might make some uses of
> compiler-specific attributes more palatable, as they can be used directly
> rather than behind a macro that is conditionally defined.
> 
> OTOH, we might still end up putting some attributes behind conditionally
> defined macros because multiple platforms provide the same feature but with
> different spellings.

Ah, I see, thanks for the clarification behind the location of the attribute changing where it applies to, that was something I'd forgotten. I'd argue that eventually biting the bullet and changing the attribute positions that we already have under macros (can be done in one fell swoop honestly) is a net gain since they now have well defined behaviour according to C++ and we know what they apply to exactly, but I don't know how much you'd agree on that point

Speaking of moving to C++17 though, what about C++20? It's the C++ version that Visual C++ gives access to the noinline and forceinline attributes which are guaranteed to not inline or always inline, as opposed to what we have now, which globalDefinitions_visCPP notes that we are uncertain whether they actually work, and while it's probably farfetched to jump an entire version of C++ just for that, I found it pretty interesting and wondered what you'd have to say about that

-------------

PR: https://git.openjdk.org/jdk/pull/12507