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

[Xiaolin Zheng]

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

Hi Lucy,

Thank you for the detailed explanation about the meaning and the history of `Disassembler::find_prev_instr()`, where I totally get for which it originally was designed, and also thanks for the understanding. Also, I feel sincerely sorry for hurting your feeling by (seemingly currently and temporarily) removing this part of the code. The x86 part is complex and TODO but other parts seem mature. In my humble opinion, one of the alternatives might be to enable the feature, giving `Disassembler::find_prev_instr()` usages to make the elaborately designed efforts alive and used in hs_err_pid files. Then x86 developers might also add support for this feature. But this might be easier said than done to me, for 'When JDK-8213084 was contributed, the changes to the signal handlers were deliberately NOT done to limit complexity and risk.', so seems a little way to go.

My opinion might be too young and too simple. In fact, I just objectively considered the maintenance issue, paying no attention to the background. I also feel pleased to retract this patch as well because it is indeed a pity for me to remove the solid code. If you have time or plan in the future to make this feature mainline-enabled, I would feel very glad to close this PR to make the work easy and add my minor contribution to its counterpart of RISC-V's 'C' extension (it would be an easy one because it is RISC).

I would be happy to hear and consider your opinion first.

Best Regards,
Xiaolin

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

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