[foreign-memaccess+abi] RFR: 8302346: Lift upcall sharing mechanism to AbstractLinker

[Jorn Vernee]

On Wed, 15 Feb 2023 15:17:26 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Lift the upcall sharing mechanism to AbstractLinker, where it can live next to the similar mechanism for down calls. This also allows the fallback linker to use this mechanism.
>> 
>> Instead of an upcall stub, AbstractLinker::arrangeDowncall now return an UpcallStubFactory, which is a callback accepting a target MethodHandle and Arena, which are then used to construct the actual upcall stub.
>> 
>> Perhaps the trickiest part of this patch is that we have to simulate the effects of `SharedUtils::adaptUpcallForIMR` on the target method type. I've added a new method in SharedUtils, called `computeUpcallIMRType` for that, which mimics the shape of the `adaptUpcallForIMR` method.
>> 
>> Since the shape of most of the arrangeUpcall methods was very similar, I factored them out into a helper method in SharedUtils as well.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 191:
> 
>> 189:         UpcallStubFactory factory = UpcallLinker.makeFactory(targetType, abi, callingSequence);
>> 190: 
>> 191:         if (isInMemoryReturn) {
> 
> Question: does this logic belong here? E.g. we have a method that is used to create an upcall factory, namely `UpcallLinker.makeFactory`, but it turns out that if we have `isInMemoryReturn` some other factory should be used instead. Shouldn't we tweak the code so that `makeFactory` always does the right thing?

It's a matter of organization. I think we should keep `UpcallLinker` ABI agnostic, and in memory returns are figments of an ABI. (note e.g. that the `CallingSequence` we pass to `makeFactory` has no notion of this in memory return. Not to be confused with the return buffer)

I see the additional step for in memory return not as using another factory, but as providing some pre-processing of the MethodHandle, before it's passed to `makeFactory`. The pre-processing is ABI-specific, so should live outside of UpcallLinker, IMO.

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

PR: https://git.openjdk.org/panama-foreign/pull/791