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
