RFR: JDK-8314163: os::print_hex_dump prints incorrectly for big endian platforms and unit sizes larger than 1 [v2]

[Aleksey Shipilev]

On Tue, 15 Aug 2023 11:44:30 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/runtime/os.cpp line 972:
>> 
>>> 970:     const int bitoffset =
>>> 971:       LITTLE_ENDIAN_ONLY(offset * BitsPerByte)
>>> 972:       BIG_ENDIAN_ONLY((int)((sizeof(intptr_t) - unitsize - offset) * BitsPerByte));
>> 
>> Um, my head hurts a bit thinking about this. So for 64-bit `sizeof(intptr_t)` = 8 and unitsize=8, `bitoffset` is negative? Which we then feed into `bitfield`, which does the shift by the negative value, which is undefined behavior?
>
> offset is based on the print address, and that one is aligned to unitsize:
> 
> https://github.com/openjdk/jdk/blob/caef3d5ac2ba34610b2fcd65045d9c9185c5de06/src/hotspot/share/runtime/os.cpp#L995
> 
> which I now also assert at the entry of print_hex_location.
> 
> For unitsize = 8, on 64-bit, print address should equal the address of the intptr_t, and the offset should be 0.
> 
> Hence also the align_as in the test: I want to make sure the address range is always aligned in a way that does not conflict with unitsize alignment.

Mhm. What worries me is that this is a common part of error handling code. `assert` does not really help us here, because callers can still hand us unaligned pointers, especially in release bits, which would face the error in this code? In other words, this code should be bullet-proof, and not rely on any externality.

One could argue that `print_hex_location` is an internal function that is protected by what `os::print_hex_dump` does. But then aligning `char buf[256]` prior to call to `os::print_hex_dump` in the test is actually a red flag! `os::print_hex_dump` should be immune to misaligned `buf`!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15256#discussion_r1294679803