RFR: 8289925: Shared code shouldn't reference the platform specific method frame::interpreter_frame_last_sp() [v3]

[Richard Reingruber]

On Thu, 15 Sep 2022 07:43:23 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>> The method `frame::interpreter_frame_last_sp()` is a platform method in the sense that it is not declared in a shared header file. It is declared and defined on some platforms though (x86 and aarch64 I think).
>> 
>> `frame::interpreter_frame_last_sp()` existed on these platforms before vm continuations (aka loom). Shared code that is part of the vm continuations implementation references it. This breaks the platform abstraction.
>> 
>> This fix simply removes the special case for interpreted frames in the shared method `Continuation::continuation_bottom_sender()`. I cannot see a reason for the distinction between interpreted and compiled frames. The shared code reference to `frame::interpreter_frame_last_sp()` is thereby eliminated.
>> 
>> Testing: hotspot_loom and jdk_loom on x86_64 and aarch64.
>
> Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Only pass the actual sp when calling is_sp_in_continuation()
>  - Merge branch 'master'
>  - Merge branch 'master'
>  - Remove platform dependent method interpreter_frame_last_sp() from shared code

Hi Dean,

I'll answer your questions below but actually I don't think it is necessary to
look into these platform details at all. The platform abstraction layer does not
specify `unextended_sp >= sp`. Consequently the callers of
is_sp_in_continuation() rely on undefined behaviour. The proposed change fixes
this.

Also if ppc64 would be changed (and maybe also aarch64 after
[JDK-8289128](https://bugs.openjdk.org/browse/JDK-8289128)?) to achieve that
unextended_sp always points into the frame then this would be just a property of
the port and, as such, not sufficient.

A dedicated RFE would be needed to establish `unextended_sp >= sp` as part of
the platform abstraction. Shared code should assert the property and it should
be covered by tests. I wouldn't be in favour of such a RFE though. It would limit
the degree of freedom without need.

> Actually, for interpreted --> interpreted, aarch64 and ppc64 seem to do the same trimming. The difference is ppc64 does it in the caller, while aarch64 does it in the callee:
> 
> https://github.com/openjdk/jdk/blob/725f41ffd4b137aef3f83700b4e181e9d93368d4/src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp#L1578
> 
> So this would mean for interpreted --> compiled, ppc64 does trimming and aarch64 does not.

I see, thanks.

> I also noticed that on ppc64, the value of unextended_sp for interpreted
> frames in inconsistent. Whether or not it is the "max stack" value depends on
> who calls the frame constructor. Some Loom code and
> sender_for_compiled_frame() sets unextended_sp the same as sp,

On x64 sender_for_compiled_frame() also sets unextended_sp == sp even though
this is not correct if the sender is an interpreted frame. It's not important
because the value is not used. At least it wasn't before Loom.

For the ppc64 Loom port I was forced to construct frames with unextended_sps
similar to x64 (e.g. pointing to the call parameters if interpreted) to satisfy
Loom shared code assumptions. Luckily it worked but IMHO that shared Loom code
breaks the platform abstraction and has to be fixed.

>  while only sender_for_interpreter_frame() uses the "max stack" value.

For good reason because if the sender is compiled then the sender_sp has to be
used as unextended_sp because that's the pointer to find 'stuff' in compiled
frames. If the sender happens to be an interpreted frame then the sender_sp will
be the "max stack" value. It would be really confusing though to use something
else. Especially if doing it only on ppc64 but not aarch64.

> Giving unextended_sp a consistent "canonical" value that is always inside the
> frame, no matter if the callee is interpreted or compiled, seems like it would
> make unextended_sp() valid for is_sp_in_continuation() as well.

Not really as it would still rely on an undefined property which, for instance,
may be invalid on riscv.

Thanks, Richard.

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

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