[foreign-abi] RFR: 8244720: Check MethodType and FunctionDescritpor used when linking

[Maurizio Cimadamore]

On Mon, 11 May 2020 10:19:39 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Hi,
> 
> This patch adds exhaustive checking to the MethodType and FunctionDescriptor used to link down calls and upcalls.
> 
> These checks define and enforce a set of acceptable carrier types, as well as which carrier type & memory layout
> combinations are acceptable. The set of accepted carrier types is:
> 1. The primitives: byte, short, char, int, long, float and double (excluding void and boolean)
> 2. MemoryAddress
> 3. MemorySegment
> 
> For (1), it is also checked that the used MemoryLayout is a ValueLayout, and that the size of the carrier matches the
> size of the layout. For (2) the expected layout must also be a ValueLayout, and again the size is checked. For (3) it
> is only checked that the layout is a GroupLayout, (since we don't have access to the size of the segment when
> linking).  This makes it easier to reason about the set of MethodType and FunctionDescriptor combinations that can be
> used during linking, as well as helping to catch any errors made with mismatching carrier types and memory layouts.
> The additional checks turned up 2 cases of carrier type to memory layout mismatch in StdLibTest, which I've fixed.
> Thanks,
> Jorn

Good fix overall - it seems like it also pointed out subtle issues in some of our tests. I've added some (optional)
suggestions about code reuse.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 201:

> 200:
> 201:     private static void checkCompatibleType(Class<?> carrier, MemoryLayout layout, long addressSize) {
> 202:         if (carrier.isPrimitive()) {

I recalled that we do this check in the foreign memory access API (see `LayoutPath.java`):

if (!carrier.isPrimitive() || carrier == void.class || carrier == boolean.class // illegal carrier?
                || Wrapper.forPrimitiveType(carrier).bitWidth() != layout.bitSize()) { // carrier has the right size?
            throw new IllegalArgumentException("Invalid carrier: " + carrier + ", for layout " + layout);
        }
Can we perhaps unify a little? It seems like your check should subsume the existing check (at least for ValueLayouts).

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

Marked as reviewed by mcimadamore (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/158