8252584: HotSpot Style Guide should permit alignas

[TheShermanTanker]

Add alignas to the permitted features set. Though the corresponding entry mentions this should not be done for classes, there's no actual difference in practice with all our supported compilers, because their nonstandard syntax also has the same limitations and issues with dynamic allocation as the C++ alignas, and including such a restriction of falling back to ATTRIBUTE_ALIGNED in the case of classes in the style guide would ultimately not really serve much of a point

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8252584: HotSpot Style Guide should permit alignas

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11315/head:pull/11315
$ git checkout pull/11315

Update a local copy of the PR:
$ git checkout pull/11315
$ git pull https://git.openjdk.org/jdk pull/11315/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11315

View PR using the GUI difftool:
$ git pr show -t 11315

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11315.diff

[bridgekeeper]

👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@TheShermanTanker The following labels will be automatically applied to this pull request:

• build
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (1813e8c0)
• 00: Full (791563f1)

[magicus]

/label -build

[openjdk]

@magicus
The build label was successfully removed.

[kimbarrett]

No. Any proposal to permit alignas needs to address questions around
"extended alignment", and especially "over-aligned types". Note that support
for extended alignment in any particular context is implementation defined.
(C++14 3.11/3)

It looks like nearly all uses of compiler-specific alignment decorations (such
as __attribute__((aligned))) in HotSpot are on variables with static
duration. I think those are probably fine. (Early gcc implementations of
alignas were pretty limited (more so than __attribute__((aligned)) from
what I found on the web), but that seems to have been fixed.)

The only exceptions I found are uses of ZCACHE_ALIGN on class data
members, which doesn't actually ensure alignment of those members!

[TheShermanTanker]

No. Any proposal to permit alignas needs to address questions around "extended alignment", and especially "over-aligned types". Note that support for extended alignment in any particular context is implementation defined. (C++14 3.11/3)

It looks like nearly all uses of compiler-specific alignment decorations (such as __attribute__((aligned))) in HotSpot are on variables with static duration. I think those are probably fine. (Early gcc implementations of alignas were pretty limited (more so than __attribute__((aligned)) from what I found on the web), but that seems to have been fixed.)

The only exceptions I found are uses of ZCACHE_ALIGN on class data members, which doesn't actually ensure alignment of those members!

I'm assuming you're referring to this when talking about extended alignment? I'm not sure if you mean this warning has to be included together in the Style Guide with alignas

An extended alignment is represented by an alignment greater than alignof(std::max_align_t). It is
implementation-defined whether any extended alignments are supported and the contexts in which they
are supported (7.6.2). A type having an extended alignment requirement is an over-aligned type. [ Note:
every over-aligned type is or contains a class type to which extended alignment applies (possibly through a
non-static data member). — end note ]

I don't think there's anything we can actually do about ZCACHE_ALIGNED, unfortunately, and the dynamic memory alignment mentioned in 8252584 is still going to be an issue regardless of whichever syntax we use (Utilizing a quick test I cobbled together):

g++ -std=c++14 -o ./align alignment.cpp

#include <cassert>
#include <cstdint>
#include <iostream>
#include <malloc.h>
#include <new>

class __attribute__((aligned(32))) AlignedVec { 
    double x, y, z;
};

int main() {
    std::cout << "sizeof(AlignedVec) is " << sizeof(AlignedVec) << '\n';
    std::cout << "alignof(AlignedVec) is " << alignof(AlignedVec) << '\n';

    auto Vec = AlignedVec{};
    auto pVec = new AlignedVec[10];

    if(reinterpret_cast<uintptr_t>(&Vec) % alignof(AlignedVec) == 0)
        std::cout << "Vec is aligned to alignof(AlignedVec)!\n";
    else
        std::cout << "Vec is not aligned to alignof(AlignedVec)!\n";

    if(reinterpret_cast<uintptr_t>(pVec) % alignof(AlignedVec) == 0)
        std::cout << "pVec is aligned to alignof(AlignedVec)!\n";
    else
        std::cout << "pVec is not aligned to alignof(AlignedVec)!\n";

    delete[] pVec;
}

./align

sizeof(AlignedVec) is 32
alignof(AlignedVec) is 32
Vec is aligned to alignof(AlignedVec)!
pVec is not aligned to alignof(AlignedVec)!

[kimbarrett]

No. Any proposal to permit alignas needs to address questions around "extended alignment", and especially "over-aligned types". [...]

I'm assuming you're referring to this when talking about extended alignment? I'm not sure if you mean this warning has to be included together in the Style Guide with alignas

An extended alignment is represented by an alignment greater than alignof(std::max_align_t). It is
implementation-defined whether any extended alignments are supported and the contexts in which they
are supported (7.6.2). A type having an extended alignment requirement is an over-aligned type. [ Note:
every over-aligned type is or contains a class type to which extended alignment applies (possibly through a
non-static data member). — end note ]

That's the paragraph I referenced. A proposal to permit the use of alignas
in HotSpot code needs to indicate where (if anywhere) the use of extended
alignment is permitted. And "never" doesn't work, since (for example) there
are a fair number of constants used by various x86 stub generators that are
given extended alignment.

Like I said, it probably works to use extended alignment for variables with
static storage duration (C++14 3.7.1) with existing platforms and build
configurations.

Variables with automatic storage duration might also work (I haven't done much
testing of that).

Even for those cases there may be limitations on what values are supported.

Heap allocation of over-aligned types does not work until C++17 (in the sense
that the requested alignment is not assured).

Types or data members of types that are never heap allocated (either directly
or by inclusion in some other type) would (probably, since we mostly don't
allow the use of C++ thread-local variables) end up falling under one of the
above. Conceptually, a type derived from StackObj or only ever included in
such would fall under the automatic variable case, but I'm pretty sure there
are places where StackObj is abused, such that it doesn't have any useful
guarantees.

So I suggest over-aligned types should be forbidden (at least until we move to
C++17).

The Style Guide should also discuss where the alignas should be placed. Some
non-local variables have it on the declaration, while others on the
definition. Perhaps it should it always be on the declaration? Or is there a
reason for that inconsistency?

I don't think there's anything we can actually do about ZCACHE_ALIGNED, unfortunately, and the dynamic memory alignment mentioned in 8252584 is still going to be an issue regardless of whichever syntax we use:

That dynamic memory alignment is the thing that is fixed by C++17.

I think none of the uses of ZCACHE_ALIGN need the requested alignment. They
are instead attempting to ensure data members are on different cache lines. We
have memory/padded.hpp for that kind of thing. I've already begun a discussion
about this with ZGC developers.

[TheShermanTanker]

No. Any proposal to permit alignas needs to address questions around "extended alignment", and especially "over-aligned types". [...]

I'm assuming you're referring to this when talking about extended alignment? I'm not sure if you mean this warning has to be included together in the Style Guide with alignas

An extended alignment is represented by an alignment greater than alignof(std::max_align_t). It is
implementation-defined whether any extended alignments are supported and the contexts in which they
are supported (7.6.2). A type having an extended alignment requirement is an over-aligned type. [ Note:
every over-aligned type is or contains a class type to which extended alignment applies (possibly through a
non-static data member). — end note ]

That's the paragraph I referenced. A proposal to permit the use of alignas in HotSpot code needs to indicate where (if anywhere) the use of extended alignment is permitted. And "never" doesn't work, since (for example) there are a fair number of constants used by various x86 stub generators that are given extended alignment.

Like I said, it probably works to use extended alignment for variables with static storage duration (C++14 3.7.1) with existing platforms and build configurations.

Variables with automatic storage duration might also work (I haven't done much testing of that).

Even for those cases there may be limitations on what values are supported.

Heap allocation of over-aligned types does not work until C++17 (in the sense that the requested alignment is not assured).

Types or data members of types that are never heap allocated (either directly or by inclusion in some other type) would (probably, since we mostly don't allow the use of C++ thread-local variables) end up falling under one of the above. Conceptually, a type derived from StackObj or only ever included in such would fall under the automatic variable case, but I'm pretty sure there are places where StackObj is abused, such that it doesn't have any useful guarantees.

So I suggest over-aligned types should be forbidden (at least until we move to C++17).

The Style Guide should also discuss where the alignas should be placed. Some non-local variables have it on the declaration, while others on the definition. Perhaps it should it always be on the declaration? Or is there a reason for that inconsistency?

I don't think there's anything we can actually do about ZCACHE_ALIGNED, unfortunately, and the dynamic memory alignment mentioned in 8252584 is still going to be an issue regardless of whichever syntax we use:

That dynamic memory alignment is the thing that is fixed by C++17.

I think none of the uses of ZCACHE_ALIGN need the requested alignment. They are instead attempting to ensure data members are on different cache lines. We have memory/padded.hpp for that kind of thing. I've already begun a discussion about this with ZGC developers.

I've modified the Style Guide to address some of the issues brought up in the review for the time being

The Style Guide should also discuss where the alignas should be placed. Some non-local variables have it on the declaration, while others on the definition. Perhaps it should it always be on the declaration? Or is there a reason for that inconsistency?

In my opinion it would be helpful to place it at both declaration and definition, just before the type specifier, so it's easier to determine that a variable has a particular alignment without having to check both. Maybe only specifying it at the declaration if that proves to be too troublesome will be enough though

Small side note: Should it also mention that ATTRIBUTE_ALIGNED will be/is deprecated after this style change, and/or that using it is equivalent to alignas? After all, as discussed above there isn't actually a difference of using either to specify alignment on whatever object or type it's desired to be used on

[kimbarrett]

n1877 is not the final version of the proposal; see n2341

[rose00]

Given that this declaration thingy has no portable meaning beyond max-align, it seems to be actually dangerous to use, for at least some of our use cases in HotSpot. And for the use cases within max-align, the usual workarounds with unions and what-not will do the job, almost always.

In short, this syntax seems like the sort of sugary flaky treat we should avoid. Making our sources less portable in order to make them more beautiful would be a bad bargain.

The ugly-but-reliable way we control stuff like is to concoct some kind of ugly macro, local to HotSpot, that has exactly the semantics that we require, and that is portable.