RFR: 8334738: os::print_hex_dump should optionally print ASCII

[David Holmes]

On Fri, 21 Jun 2024 16:17:43 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> 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.

I don't follow - you didn't change/add any callsites so why would you need to cast away constness to "feed" the existing call ?? 

More comments below.

I like the idea in general.

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

> 942: }
> 943: 
> 944: ATTRIBUTE_NO_ASAN static bool read_safely_from(const uint8_t* p, uintptr_t* result) {

I don't understand the change to `uint8_t` - that is just an unsigned char, so casting to `intptr_t` we could have a misaligned value. ??

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

> 959:   union {
> 960:     uint64_t v;
> 961:     uint8_t c[sizeof(v)];

Why `uint8_t` instead of `unsigned char`?

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

> 964:     const int idx = LITTLE_ENDIAN_ONLY(i) BIG_ENDIAN_ONLY(sizeof(u.v) - 1 - i);
> 965:     const uint8_t c = u.c[idx];
> 966:     ascii_form.put(isprint(c) && isascii(c) ? c : '_');

Isn't it customary to print a `?` if the char is unprintable?

src/hotspot/share/runtime/os.hpp line 860:

> 858:   static void print_hex_dump(outputStream* st, const uint8_t* start, const uint8_t* end, int unitsize, bool print_ascii,
> 859:                              int bytes_per_line, const uint8_t* logical_start);
> 860:   static void print_hex_dump(outputStream* st, const uint8_t* start, const uint8_t* end, int unitsize, bool print_ascii = true) {

If you have made this a default parameter why did you change so many call-sites to pass `true`?

Can I suggest when you do pass true/false you annotate it as below e.g. `true /* print_ascii */`

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

PR Review: https://git.openjdk.org/jdk/pull/19835#pullrequestreview-2137506667
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652090760
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652092990
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652093561
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652100163