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

[Thomas Stuefe]

On Tue, 25 Jun 2024 07:05:47 GMT, David Holmes <dholmes 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 ??
> 

Mostly in tests. I rewrote them several times, one version was fed from const memory. 

If this aspect turns out to be contentious, I'll revert it. I still think this function should accept const pointers as input.

I chose uint8_t since it seemed to me the closest const version of address. address, however, is signed char, but I did not want to pass const char* since that looks like a string.

I can use unsigned char*, sure. Or, maybe just const void*. Both are fine to me.

> More comments below.
> 
> I like the idea in general.

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

It depends; I also know hex dumps with dot (.) and blanks. ? It looks a bit noisy, visually. I think _ or . or blank make the real ascii portions stick out better.

I can live with anything, but would prefer dot or underscore. Dot seems to be the most common, thinking about it.

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

Good catch. I'll revert the call site changes and leave the default parameter.

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

Sure.

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

PR Comment: https://git.openjdk.org/jdk/pull/19835#issuecomment-2188273005
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652234333
PR Review Comment: https://git.openjdk.org/jdk/pull/19835#discussion_r1652238732