RFR (S) 8217879: hs_err should print more instructions in hex dump

[Thomas Stüfe]

Hi Alexey,

I agree that a larger dump would be helpful, I had the same thought in the
past. I know others prefer slim hs-err files, but they would have to chime
in.

Bikeshedding: I do not like naming that thing os::print_instructions(),
since we do not do that, we just print a hex dump. Especially on x86, where
the "unitsize" parameter is confusing because we have a variable sizes
instruction set. I would prefer something like this:

os::print_hex_dump_surrounding(address pivot, size_t len, size_t unitsize)

and let that function print the pivot address up front (obviously avoiding
to name it "pc"), then the dump (+- len around pivot) and maybe a little
">" marker at the start of the pivot line.

--

In addition, I never liked that "os::print_context" even calls this. The
function name sounds harmless, like a simple "print the ucontext structure"
but is potentially dangerous since it just dereferences whatever it finds
in pc. So even though it looks that way it is by no means a general purpose
function, but only usable in error reporting. Your patch makes it more
dangerous since the printed area now is larger and so we run a larger risk
in segfaulting.

I have a version of os::print_hex_dump locally somewhere which uses
SafeFetch32 to print out the hex dump, printing little "?" for unmapped
memory. I think that would be the correct way. Also I think that the
print-instructions and print-top-pf-stack parts should be removed from the
os::print_context() and called directly from vmError.cpp.

But I understand if you want to leave this improvement to some other time.

---

Patch looks fine otherwise.

Cheers, Thomas




On Mon, Jan 28, 2019 at 5:50 PM Aleksey Shipilev <shade at redhat.com> wrote:

> RFE:
>   https://bugs.openjdk.java.net/browse/JDK-8217879
>
> Fix:
>   http://cr.openjdk.java.net/~shade/8217879/webrev.01/
>
> "Instructions" block is useful when following up on hs_errs that happened
> without the disassembler
> attached, which is usually the case coming from users. One can use the
> disassembler [1] to look
> around the code that was crashing, and get extended conjectures why the
> error happened, including
> rewinding a bit of history. However, current window is sometimes too small
> to infer enough context.
> I propose we extend it!
>
> The patch also commons the paths across OS/Arch-specific files to that
> current "delta" appears less
> of the magic number. Plus, it adds cr()-s for consistency across the
> arches and within the methods.
>
> Testing: eyeballing hs_errs from artificial crashes, Linux x86_64 build,
> jdk-submit
>
> Thanks,
> -Aleksey
>
> [1] I use https://onlinedisassembler.com, for example.
>
>