[foreign-memaccess+abi] RFR: 8292037: Consider moving Linker::methodType as FunctionDescriptor::toMethodType [v2]

[Maurizio Cimadamore]

On Thu, 8 Sep 2022 17:02:58 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch moves `Linker::methodType` to `FunctionDescriptor` as `carrierMethodType`.
>> 
>> I went with the name `carrierMethodType` instead of `toMethodType` (as suggested by the JBS issue) since I think it's a bit more descriptive. Please let me know your thoughts.
>> 
>> This issue also suggest adding support for `SequenceLayout` in the linker implementation. I think adding support in FunctionDescriptor could be a good idea, but since passing arrays by value is not supported in C, I feel like we should reject it in the linker implementations we have (which are all C as well). Of course, we could make it work any way by interpreting such cases as a pointer layout instead, but it's a case of being able to do the same thing in multiple ways (which I think can be confusing), and might actually be surprising if someone passes the wrong layout to the linker by accident, but it doesn't get "caught".
>> 
>> I've added support for `SequenceLayout` to `FunctionDescriptor`, but it is still rejected in the type classification logic.
>
> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add some simple tests

I think for now your choice is ok. Depending on whether we land with GroupLayout we might need to revisit though. Note that a sequence layout is just another way to describe a struct layout - so I don't think (as you seem to suggest) that we should pass by pointer, but we should pass by value instead (expanding the sequence layout to the corresponding struct layout).

So, if GroupLayout only covers structs and unions (as now), what we have now makes sense. If GroupLayout is enhanced to cover all composite layouts (including SequenceLayout), then I think the current position seems a tad more inconsistent, as one might like to be able to use GroupLayout for all things that I can pass by value to the linker.

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 105:

> 103:      * <li>If the layout is a {@link ValueLayout} the carrier type is determined through {@link ValueLayout#carrier()}.</li>
> 104:      * <li>If the layout is a {@link GroupLayout}, or {@link SequenceLayout} the carrier type is {@link MemorySegment}.</li>
> 105:      * <li>If the layout is a {@link PaddingLayout} an {@link IllegalArgumentException} is thrown.</li>

This comment seems to be inconsistent with the @throws clause

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 112:

> 110:      * (e.g. if they are sequence layouts or padding layouts).
> 111:      */
> 112:     MethodType carrierMethodType();

this name seems long, and the extra wording doesn't seem to add a lot (at least IMHO).

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

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