RFR: 8334738: os::print_hex_dump should optionally print ASCII [v3]

[Severin Gehwolf]

On Thu, 27 Jun 2024 08:05:42 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Motivated by analyzing CDS dump differences for reproducible builds, I found an optional ASCII printout to be valuable. As usual with hex dumps, ascii follows hex printout
>> 
>> Example:
>> 
>> 
>> 
>>    118 0x00000000000001c0:   204b444a6e65704f 53207469422d3436 4d56207265767265 6564747361662820   OpenJDK 64-Bit Server VM (fastde
>>    119 0x00000000000001e0:   692d343220677562 2d6c616e7265746e 68742e636f686461 756f732e73616d6f   bug 24-internal-adhoc.thomas.sou
>>    120 0x0000000000000200:   726f662029656372 612d78756e696c20 45524a203436646d 746e692d34322820   rce) for linux-amd64 JRE (24-int
>>    121 0x0000000000000220:   64612d6c616e7265 6d6f68742e636f68 6372756f732e7361 6c697562202c2965   ernal-adhoc.thomas.source), buil
>>    122 0x0000000000000240:   323032206e6f2074 5430322d36302d34 32313a35343a3031 672068746977205a   t on 2024-06-20T10:45:12Z with g
>>    123 0x0000000000000260:   2e352e3031206363 0000000000000030 0000000000000000 0000000000000000   cc 10.5.0_______________________
>>    124 0x0000000000000280:   0000000000000000 0000000000000000 0000000000000000 0000000000000000   ________________________________
>>    125 0x00000000000002a0:   0000000000000000 0000000000000000 0000000000000000 0000000000000000   ________________________________
>> 
>> 
>> The patch does that.
>> 
>> Small unrelated changes:
>> 
>> - I rewrote and extended the gtests, testing now a real-life printout containing a mixture or readable and non-readable pages, and printable and non-printable characters. I re-enabled tests on Windows, since https://bugs.openjdk.org/browse/JDK-8185734 is long solved.
>> 
>> - The new test uncovered an issue on 32-bit when printing giant words. We shift a signed value by 32 bits upwards, which can result in -1 resp. ffffffff in the upper half of the giant word. One of the pitfalls of intptr_t vs uintptr_t (I think most uses of intptr_t should probably use uintptr_t).
>> 
>> - I got tired of casting constness away from to-be-printed memory range just to be able to feed an address to os::print_hex_dump. The content printed is usually const. os::print_hex_dump does not need non-constness, but since we use address, and address is typedef char*, and one cannot declare a typedef'ed pointer target-const, the issue is there. I therefore changed the input to const uint8_t*. Maybe we need a const_address or something similar.
>> 
>> ----
>> 
>> Ran tests on Linux x64 and x86, Windows x86 and Mac aarch64. Fixed all issues I found. Only little-endian, I don't have big-e...
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   exclude test for AIX

This isn't really the area of my expertise, but the patch seems reasonable to me.

src/hotspot/share/runtime/os.cpp line 945:

> 943: 
> 944: ATTRIBUTE_NO_ASAN static bool read_safely_from(const uintptr_t* p, uintptr_t* result) {
> 945:   DEBUG_ONLY(*result = 0xAAAA;)

It's not clear why this was added. Left-over?

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

Marked as reviewed by sgehwolf (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19835#pullrequestreview-2156999448
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1664527314