RFR: 8284433: Cleanup Disassembler::find_prev_instr() on all platforms

[Lutz Schmidt]

On Wed, 6 Apr 2022 08:06:17 GMT, Xiaolin Zheng <xlinzheng at openjdk.org> wrote:

> Hi team,
> 
> This is a trivial cleanup for `Disassembler::find_prev_instr()` on all platforms. This function is used nowhere and seems to be loaded in JDK-13 (JDK-8213084) [1]. I looked through the code and found it very solid that it checks many corner cases (unreadable pages on s390x, etc.). But it is not used so modifying and testing it might be hard, especially the corner cases that could increase burdens to maintenance. On RISC-V we also need to make the current semantics of this function[2] right (Thanks to Felix @RealFYang pointing out that) because there is an extension 'C' that can compress the size of some instructions from 4-byte to 2-byte. Though I have written one version on my local branch, I began to think if removing this might be a better choice anyway.
> 
> Tested by building hotspot on x86, x86-zero, AArch64, RISC-V 64, and s390x. (I don't have access to s390x and ppc real machines, but have an s390x sysroot and qemu)
> 
> (I feel also pleased to retract this patch if there are objections.)
> 
> [1] https://github.com/openjdk/jdk13u-dev/commit/b730805159695107fbf950a0ef48e6bed5cf5bba
> [2] https://github.com/openjdk/jdk/blob/0a67d686709000581e29440ef13324d1f2eba9ff/src/hotspot/cpu/riscv/disassembler_riscv.hpp#L38-L44
> 
> Thanks,
> Xiaolin

As the name suggests, find_prev_instr() has the purpose of stepping backwards in the instruction stream. The task can be accomplished rather easily on architectures with a fixed instruction length (mostly RISC architectures). For CISC architectures (x86 and s390 for the scope of HotSpot), the task is significantly more complex. For s390, complexity is kept in check because the length of each instruction is coded in the leftmost two bits of the instruction. That allows for straightforward forward stepping. For x86, however, I assume you need to write a full instruction decoder even to step forward. 

So why is there a need for find_prev_instr() after all? 
It's pure convenience. Imagine the VM catches a signal (SIGSEGV, SIGILL, ...). A hs_err_pid file is written, containing a memory snippet around the location where the signal occurs. The memory snippet is dumped twice, once as "classical" hex dump and once as (abstract) disassembly. In case a suitable disasembler library is available, you get a nice disassembly from around the problematic instruction.

The current implementation in HotSpot only provides a disassembly forward from the failing instruction without taking advantage of find_prev_instr(). When JDK-8213084 was contributed, the changes to the signal handlers were deliberately NOT done to limit complexity and risk.

But that's all history. If you feel like getting rid of this unused code, go ahead. Should I (or somebody else) find time in the future to enhance hs_err file disassembly output, I can easily re-contribute the function.

Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/8120