RFR: 8337662: Improve os::print_hex_dump for printing Instructions sections

[Thomas Stuefe]

On Thu, 1 Aug 2024 13:13:40 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

> Currently we use os::print_hex_dump for printing Instruction sections in the hserr file. The function could be slightly improved, .e.g. by showing directly the pc .
> The instructions section would for example look like this, with a small helper '=>' pointing to the pc. Makes the output more readable.

Hi Matthias,

this is useful.

We need some gtests for this:
- test with logical_address = nullptr, and highlighted address at the start/middle/end of a hex line
- test with logical_address != nullptr, and highlighted address should then correspond to the logical address

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

> 1025:   // highlight_address should be within the printed range
> 1026:   if (highlight_address != nullptr) {
> 1027:     if (highlight_address < start || highlight_address > end) {

Range is `[)`, so it should be `highlight_address >= end`

But I would just assert in debug builds, not do anything in release builds:

Its an error - the caller is responsible for giving us a highlight address that is in range. So in debug builds we get an error. But the error is benign, since all it causes is the function ignoring the highlight hint - since it never encounters the address. Therefore, setting it to NULL is superfluous.

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

> 1046:     if (cols == 0) {
> 1047:       // highlight start of line if address of interest is located there
> 1048:       if (highlight_address != nullptr && highlight_address == p) {

This only works if the highlighted address happens to fall to the start of a printed hex dump line, or? If it falls into the middle of the line, it will fail. E.g. :  `os::print_hex_dump(st, 0x10000000, 0x10000200, 8, false, 32, 0x10000008);`.

So, what you should do is to test if the highlight address falls into the range the printed line covers.

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

> 1054:         } else {
> 1055:           st->print(PTR_FORMAT ":   ", p2i(logical_p));
> 1056:         }

proposal for less redundancy:

const char* const prefix = 
  (highlight_address != nullptr) ? (should_highlight ? "=>" : "  ") : "";
st->print("%s" PTR_FORMAT ":   ", prefix, p2i(logical_p));

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

Changes requested by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20419#pullrequestreview-2212939218
PR Review Comment: https://git.openjdk.org/jdk/pull/20419#discussion_r1700279476
PR Review Comment: https://git.openjdk.org/jdk/pull/20419#discussion_r1700267804
PR Review Comment: https://git.openjdk.org/jdk/pull/20419#discussion_r1700295537