On Wed, 15 Feb 2023 20:38:04 GMT, Johannes Bechberger <d...@openjdk.org> wrote:

>>> I think `unextended_sp < sp` is possible on x86 when interpreter entries 
>>> generated by `MethodHandles::generate_method_handle_interpreter_entry()` 
>>> are executed. They modify `sp` ([here for 
>>> example](https://github.com/openjdk/jdk/blob/7f71a1040d9c03f72d082e329ccaf2c4a3c060a6/src/hotspot/cpu/x86/methodHandles_x86.cpp#L310-L314))
>>>  but not `sender_sp` (`InterpreterMacroAssembler ::_bcp_register`).
>> 
>> I think we're talking about a case where an interpreted caller calls an 
>> interpreted callee through a MethodHandle. If my understanding is correct 
>> (looking at `frame::sender_for_interpreter_frame`), in that case there are 2 
>> sender sps, one that is computed based on the frame pointer of the callee. 
>> This just adds 2 words (skipping the return address), and the other is the 
>> sp which is saved into `r13` in 
>> `InterpreterMacroAssembler::prepare_to_jump_from_interpreted`, and before 
>> e.g. a c2i adapter extends the stack 
>> [here](https://github.com/openjdk/jdk/blob/50dcc2aec5b16c0826e27d58e49a7f55a5f5ad38/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L631)
>>  Which then seems to be saved in the interpreted callee's frame as well (I'm 
>> not super familiar with interpreter frame layouts though...). The latter 
>> being what ends up as `unextended_sp`.
>> 
>> I think @reinrich's conclusion is correct wrt `unextended_sp < sp` in that 
>> case. For certain MH linker calls, an additional `MemberName` 'appendix' is 
>> passed as the last argument. The MH adapter/trampoline stub pops the 
>> appendix argument from the stack for an interpreted caller, and uses it to 
>> find out the actual callee. But that means that, the sp saved into `r13` 
>> will be one slot too 'large' (small? accounting for one too many arguments). 
>> In other words, the MethodHandle adapter for interpreted callers 'consumes' 
>> the appendix argument, but doesn't adjust `r13`. I think this could be done 
>> though, i.e. just add:
>> 
>> 
>>  __ addptr(r13, Interpreter::stackElementSize);   // adjust unexteded_sp for 
>> callee, for proper stack walking
>> 
>> 
>> In the linked code in 
>> `MethodHandles::generate_method_handle_interpreter_entry` to make this work.
>> 
>> (note that we can not just leave the appendix on the stack, since the callee 
>> doesn't expect it, so we would break argument oop GC if we left it there)
>> 
>> If that works, I think doing that would be preferable to adjusting 
>> `safe_for_sender`. That seems like it would uphold the `sp <= unextended_sp` 
>> invariant as well (maybe an assert to that effect in the constructor of 
>> `frame` wouldn't be a bad idea as well).
>> 
>> It seems like PPC is not modifying the stack in this case (?):
>> 
>> 
>>     Register R19_member = R19_method;  // MemberName ptr; incoming method 
>> ptr is dead now
>>     __ ld(R19_member, RegisterOrConstant((intptr_t)8), R15_argbase);
>>     __ addi(R15_argbase, R15_argbase, Interpreter::stackElementSize);
>>     generate_method_handle_dispatch(_masm, iid, tmp_recv, R19_member, 
>> not_for_compiler_entry);
>
> This is an interesting insight, I'm going to test your fix idea tomorrow.

> I think your conclusion is correct wrt `unextended_sp < sp` in that case. For 
> certain MH linker calls, an additional `MemberName` 'appendix' is passed as 
> the last argument. The MH adapter/trampoline stub pops the appendix argument 
> from the stack for an interpreted caller, and uses it to find out the actual 
> callee. But that means that, the sp saved into `r13` will be one slot too 
> 'large' (small? accounting for one too many arguments). In other words, the 
> MethodHandle adapter for interpreted callers 'consumes' the appendix 
> argument, but doesn't adjust `r13`. I think this could be done though, i.e. 
> just add:
> 
> ```
>  __ addptr(r13, Interpreter::stackElementSize);   // adjust unexteded_sp for 
> callee, for proper stack walking
> ```

I don't think you can do that. Modifying the unextended_sp would break accesses 
to compiled frames (gc, locals). The original unextended_sp is also needed to 
undo the c2i extension.

> In the linked code in 
> `MethodHandles::generate_method_handle_interpreter_entry` to make this work.
> 
> (note that we can not just leave the appendix on the stack, since the callee 
> doesn't expect it, so we would break argument oop GC if we left it there)
> 
> If that works, I think doing that would be preferable to adjusting 
> `safe_for_sender`. That seems like it would uphold the `sp <= unextended_sp` 
> invariant as well (maybe an assert to that effect in the constructor of 
> `frame` wouldn't be a bad idea as well).
> 
> It seems like PPC is not modifying the stack in this case (?):
> 
> ```
>     Register R19_member = R19_method;  // MemberName ptr; incoming method ptr 
> is dead now
>     __ ld(R19_member, RegisterOrConstant((intptr_t)8), R15_argbase);
>     __ addi(R15_argbase, R15_argbase, Interpreter::stackElementSize);
>     generate_method_handle_dispatch(_masm, iid, tmp_recv, R19_member, 
> not_for_compiler_entry);
> ```

Only the expression stack pointer (R15) is modified but not the sp (R1).

unextended_sp < sp is possible on PPC because the expression stack is allocated 
with its maximum size and it's trimmed to the used part when making a call. The 
sender sp is the original sp.

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

PR: https://git.openjdk.org/jdk/pull/12535

Reply via email to