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

[David Holmes]

On Tue, 25 Jun 2024 12:45:22 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 five additional commits since the last revision:
> 
>  - copyrights
>  - const_address instead of const uint8_t*
>  - use dot instead of underscore for unprintable
>  - rely on default true
>  - Revert "fix copyrights"
>    
>    This reverts commit 2b8bc55e53a88d13cd268dc89ebaac7fe42f60d5.

Okay - seems reasonable.

FYI I am away for a few days.

Thanks

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

> 947:   uintptr_t i = (uintptr_t)SafeFetchN((intptr_t*)p, errval);
> 948:   if (i == errval) {
> 949:     i = (uintptr_t)SafeFetchN((intptr_t*)p, ~errval);

Pre-existing but if the initial fetch fails why do we think the second one can succeed ???

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

> 962:     uint8_t c[sizeof(v)];
> 963:   } u = { value };
> 964:   for (int i = 0; i < unitsize; i ++) {

Nit: no space before unary operator ++

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19835#pullrequestreview-2144419073
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1656562653
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1656564416