RFR: 8369828: Generalize share/utilities/bytes.hpp [v2]

[Kim Barrett]

On Tue, 14 Oct 2025 15:12:13 GMT, Justin King <jcking at openjdk.org> wrote:

>> Implement portable `share/utilities/bytes.hpp` and remove CPU-specific headers.
>
> Justin King has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Qualify identifiers
>   
>   Signed-off-by: Justin King <jcking at google.com>

Changes requested by kbarrett (Reviewer).

src/hotspot/share/utilities/bytes.hpp line 81:

> 79: 
> 80:     if (Endian::is_Java_byte_ordering_different()) {
> 81:       x = byteswap(x);

If a little-endian platform doesn't support unaligned accesses, this
restucturing results in this being a byteswap of a byte-by-byte copied value.
Are compilers smart enough to merge those? I think affected platforms are arm,
riscv64, and zero. The old arm and zero code avoids calling byteswap. The old
riscv64 does just use the byteswap approach.

src/hotspot/share/utilities/unalignedAccess.hpp line 53:

> 51:   static void store(void* ptr, T value) {
> 52:     STATIC_ASSERT(std::is_trivially_copyable<T>::value);
> 53:     STATIC_ASSERT(std::is_trivially_default_constructible<T>::value);

I'm not seeing why we should care about trivially default-constructible here?

src/hotspot/share/utilities/unalignedAccess.hpp line 61:

> 59:   static T load(const void* ptr) {
> 60:     STATIC_ASSERT(std::is_trivially_copyable<T>::value);
> 61:     STATIC_ASSERT(std::is_trivially_default_constructible<T>::value);

Don't use `STATIC_ASSERT` in new code; use `static_assert`. C++17 changed `static_assert` to
support uses without a message string.

src/hotspot/share/utilities/unalignedAccess.hpp line 143:

> 141:   }
> 142: };
> 143: #endif

Please add a comment about what condition is being closed.

src/hotspot/share/utilities/unalignedAccess.hpp line 146:

> 144: 
> 145: template<size_t byte_size>
> 146: struct UnalignedAccess::StoreImpl {

These general implementations never get used in the ASAN-specialized case.  Maybe the `#endif`
a few lines back should be an `#else` instead, and just suppress these general implementations
in that case.

src/hotspot/share/utilities/unalignedAccess.hpp line 155:

> 153:     // the actual call to memcpy. On platforms which allow unaligned access,
> 154:     // the compiler will emit a normal store instruction.
> 155:     memcpy(ptr, &value, sizeof(T));

In slowdebug builds this will be pretty terrible, but presumably there aren't enough uses of these
functions for that to matter.

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

PR Review: https://git.openjdk.org/jdk/pull/27801#pullrequestreview-3336164044
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2430011521
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2430032532
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2429529774
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2429967249
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2430004064
PR Review Comment: https://git.openjdk.org/jdk/pull/27801#discussion_r2430035937