RFR: 8283737: riscv: MacroAssembler::stop() should emit fixed-length instruction sequence

[Fei Yang]

On Mon, 28 Mar 2022 04:41:11 GMT, Xiaolin Zheng <xlinzheng at openjdk.org> wrote:

> Hi team,
> 
> Could I have a review of this simple patch? -
> 
> `PhaseOutput::fill_buffer` detects if the real size of a node matches (<=) the size of it in scratch_emit(). The call chain for MacroAssembler::stop() is:
> 
> 
> MachEpilogNode::emit
>     -> reserved_stack_check()
>          -> should_not_reach_here()
>               -> stop(const char *msg)
> 
> 
> `li()` on RISCV could generate 1~6 instructions, and the msg argument could be an on-stack buffer; `stop()` also uses `__ pc()` that could also be different in `scratch_emit()` and `emit()`. They both have the potential issue here so the size generated in `MacroAssembler::stop()` needs to be a fixed value.
> 
> Could be reproduced in the fastdebug build by adding one line:
> 
> 
> // Die now.
> instruct ShouldNotReachHere() %{
>   match(Halt);
>   ins_cost(BRANCH_COST);
>   format %{ "#@ShouldNotReachHere" %}
>   ins_encode %{
>     Assembler::CompressibleRegion cr(&_masm);
>     if (is_reachable()) {
>       __ halt();
> +     __ unimplemented("this is an on-stack char literal");  // assertion fail at 'assert(false, "wrong size of mach node");'
>     }
>   %}
>   ins_pipe(pipe_class_default);
> %}
> 
> 
> This patch also fixes a typo introduced in JDK-8278994: `c_bnez` is mistakenly written to `c_beqz`, though not used until now, needing a fix for future usage.
> 
> 
> Tests passed in hotspot tier1 & jdk tier1 without new errors found.
> 
> Thanks,
> Xiaolin

Changes requested by fyang (Committer).

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 532:

> 530: 
> 531: void MacroAssembler::stop(const char* msg) {
> 532:   // these shall generate fixed-length instruction sequences

Looks reasonable to me. (Not a JDK Reviewer)
Suggestion for the code comment:

@@ -531,6 +531,9 @@ void MacroAssembler::resolve_jobject(Register value, Register thread, Register t
 void MacroAssembler::stop(const char* msg) {
   address ip = pc();
   pusha();
+  // The length of the instruction sequence emitted should be independent
+  // of the values of msg and ip so that size of mach node for scratch emit
+  // and normal emit matches.
   li(c_rarg0, (uintptr_t)(address)msg);
   li(c_rarg1, (uintptr_t)(address)ip);
   mv(c_rarg2, sp);

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

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