RFR: JDK-8314163: os::print_hex_dump prints incorrectly for big endian platforms and unit sizes larger than 1 [v2]
Aleksey Shipilev
shade at openjdk.org
Tue Aug 15 16:28:08 UTC 2023
On Tue, 15 Aug 2023 15:29:52 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> 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`!
>
>> 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?
>
> We always align down the pointer by unit size: https://github.com/openjdk/jdk/blob/caef3d5ac2ba34610b2fcd65045d9c9185c5de06/src/hotspot/share/runtime/os.cpp#L995
>
> The assert in print_hex_location (a local static function, not externally exposed) just asserts this. More for documentation purposes really.
>
>> But then aligning char buf[256] prior to call to os::print_hex_dump in the test is actually a red flag!
>
> When I wrote the test originally, I allocated the input buffer with malloc() in order to have a guaranteed alignment. But I added explicit tests for crooked aligned pointers:
>
> https://github.com/openjdk/jdk/blame/caef3d5ac2ba34610b2fcd65045d9c9185c5de06/test/hotspot/gtest/runtime/test_os.cpp#L237
>
> I'm nothing if not thorough :)
>
> About the alignas, that was a stupid thinking error for multiple reasons and I'll remove it again (its completely pointless).
All right then. Remove `alignas`, and we are good.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15256#discussion_r1294844829
More information about the hotspot-runtime-dev
mailing list