8307253: Make FunctionDescriptor::toMethodType an instance method of Linker

Reviewed-by: jvernee

Author: mcimadamore

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java

@@ -62,6 +62,7 @@ public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {
      * Returns a function descriptor with the given argument layouts appended to the argument layout array
      * of this function descriptor.
      * @param addedLayouts the argument layouts to append.
+     * @throws IllegalArgumentException if one of the layouts in {@code addedLayouts} is a padding layout.
      * @return the new function descriptor.
      */
     FunctionDescriptor appendArgumentLayouts(MemoryLayout... addedLayouts);
@@ -72,13 +73,15 @@ public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {
      * @param index the index at which to insert the arguments
      * @param addedLayouts the argument layouts to insert at given index.
      * @return the new function descriptor.
+     * @throws IllegalArgumentException if one of the layouts in {@code addedLayouts} is a padding layout.
      * @throws IllegalArgumentException if {@code index < 0 || index > argumentLayouts().size()}.
      */
     FunctionDescriptor insertArgumentLayouts(int index, MemoryLayout... addedLayouts);
 
     /**
      * Returns a function descriptor with the given memory layout as the new return layout.
      * @param newReturn the new return layout.
+     * @throws IllegalArgumentException if {@code newReturn} is a padding layout.
      * @return the new function descriptor.
      */
     FunctionDescriptor changeReturnLayout(MemoryLayout newReturn);
@@ -96,10 +99,12 @@ public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {
      * The carrier type of a layout is determined as follows:
      * <ul>
      * <li>If the layout is a {@link ValueLayout} the carrier type is determined through {@link ValueLayout#carrier()}.</li>
-     * <li>If the layout is a {@link GroupLayout} the carrier type is {@link MemorySegment}.</li>
-     * <li>If the layout is a {@link PaddingLayout}, or {@link SequenceLayout} an {@link IllegalArgumentException} is thrown.</li>
+     * <li>If the layout is a {@link GroupLayout} or a {@link SequenceLayout}, the carrier type is {@link MemorySegment}.</li>
      * </ul>
      *
+     * @apiNote A function descriptor cannot, by construction, contain any padding layouts. As such, it is not
+     * necessary to specify how padding layout should be mapped to carrier types.
+     *
      * @return the method type consisting of the carrier types of the layouts in this function descriptor
      * @throws IllegalArgumentException if one or more layouts in the function descriptor can not be mapped to carrier
      *                                  types (e.g. if they are sequence layouts or padding layouts).
@@ -110,6 +115,8 @@ public sealed interface FunctionDescriptor permits FunctionDescriptorImpl {
      * Creates a function descriptor with the given return and argument layouts.
      * @param resLayout the return layout.
      * @param argLayouts the argument layouts.
+     * @throws IllegalArgumentException if {@code resLayout} is a padding layout.
+     * @throws IllegalArgumentException if one of the layouts in {@code argLayouts} is a padding layout.
      * @return the new function descriptor.
      */
     static FunctionDescriptor of(MemoryLayout resLayout, MemoryLayout... argLayouts) {
@@ -121,6 +128,7 @@ static FunctionDescriptor of(MemoryLayout resLayout, MemoryLayout... argLayouts)
     /**
      * Creates a function descriptor with the given argument layouts and no return layout.
      * @param argLayouts the argument layouts.
+     * @throws IllegalArgumentException if one of the layouts in {@code argLayouts} is a padding layout.
      * @return the new function descriptor.
      */
     static FunctionDescriptor ofVoid(MemoryLayout... argLayouts) {

src/java.base/share/classes/jdk/internal/foreign/FunctionDescriptorImpl.java

@@ -28,6 +28,8 @@
 import java.lang.foreign.GroupLayout;
 import java.lang.foreign.MemoryLayout;
 import java.lang.foreign.MemorySegment;
+import java.lang.foreign.PaddingLayout;
+import java.lang.foreign.SequenceLayout;
 import java.lang.foreign.ValueLayout;
 import java.lang.invoke.MethodType;
 import java.util.ArrayList;
@@ -47,6 +49,13 @@ public final class FunctionDescriptorImpl implements FunctionDescriptor {
     private final List<MemoryLayout> argLayouts;
 
     private FunctionDescriptorImpl(MemoryLayout resLayout, List<MemoryLayout> argLayouts) {
+        if (resLayout instanceof PaddingLayout) {
+            throw new IllegalArgumentException("Unsupported padding layout return in function descriptor: " + resLayout);
+        }
+        Optional<MemoryLayout> paddingLayout = argLayouts.stream().filter(l -> l instanceof PaddingLayout).findAny();
+        if (paddingLayout.isPresent()) {
+            throw new IllegalArgumentException("Unsupported padding layout argument in function descriptor: " + paddingLayout.get());
+        }
         this.resLayout = resLayout;
         this.argLayouts = List.copyOf(argLayouts);
     }
@@ -120,10 +129,11 @@ public FunctionDescriptorImpl dropReturnLayout() {
     private static Class<?> carrierTypeFor(MemoryLayout layout) {
         if (layout instanceof ValueLayout valueLayout) {
             return valueLayout.carrier();
-        } else if (layout instanceof GroupLayout) {
+        } else if (layout instanceof GroupLayout || layout instanceof SequenceLayout) {
             return MemorySegment.class;
         } else {
-            throw new IllegalArgumentException("Unsupported layout: " + layout);
+            // Note: we should not worry about padding layouts, as they cannot be present in a function descriptor
+            throw new AssertionError("Cannot get here");
         }
     }
 

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java

@@ -36,7 +36,6 @@
 import jdk.internal.foreign.layout.AbstractLayout;
 import jdk.internal.reflect.CallerSensitive;
 import jdk.internal.reflect.Reflection;
-import jdk.internal.foreign.layout.ValueLayouts;
 
 import java.lang.foreign.AddressLayout;
 import java.lang.foreign.GroupLayout;
@@ -130,11 +129,20 @@ public SystemLookup defaultLookup() {
     protected abstract ByteOrder linkerByteOrder();
 
     private void checkLayouts(FunctionDescriptor descriptor) {
-        descriptor.returnLayout().ifPresent(this::checkLayoutsRecursive);
-        descriptor.argumentLayouts().forEach(this::checkLayoutsRecursive);
+        descriptor.returnLayout().ifPresent(this::checkLayout);
+        descriptor.argumentLayouts().forEach(this::checkLayout);
     }
 
-    private void checkLayoutsRecursive(MemoryLayout layout) {
+    private void checkLayout(MemoryLayout layout) {
+        // Note: we should not worry about padding layouts, as they cannot be present in a function descriptor
+        if (layout instanceof SequenceLayout) {
+            throw new IllegalArgumentException("Unsupported layout: " + layout);
+        } else {
+            checkLayoutRecursive(layout);
+        }
+    }
+
+    private void checkLayoutRecursive(MemoryLayout layout) {
         checkHasNaturalAlignment(layout);
         if (layout instanceof ValueLayout vl) {
             checkByteOrder(vl);
@@ -145,7 +153,7 @@ private void checkLayoutsRecursive(MemoryLayout layout) {
                 // check element offset before recursing so that an error points at the
                 // outermost layout first
                 checkMemberOffset(sl, member, lastUnpaddedOffset, offset);
-                checkLayoutsRecursive(member);
+                checkLayoutRecursive(member);
 
                 offset += member.bitSize();
                 if (!(member instanceof PaddingLayout)) {
@@ -156,14 +164,14 @@ private void checkLayoutsRecursive(MemoryLayout layout) {
         } else if (layout instanceof UnionLayout ul) {
             long maxUnpaddedLayout = 0;
             for (MemoryLayout member : ul.memberLayouts()) {
-                checkLayoutsRecursive(member);
+                checkLayoutRecursive(member);
                 if (!(member instanceof PaddingLayout)) {
                     maxUnpaddedLayout = Long.max(maxUnpaddedLayout, member.bitSize());
                 }
             }
             checkGroupSize(ul, maxUnpaddedLayout);
         } else if (layout instanceof SequenceLayout sl) {
-            checkLayoutsRecursive(sl.elementLayout());
+            checkLayoutRecursive(sl.elementLayout());
         }
     }
 

test/jdk/java/foreign/TestFunctionDescriptor.java

@@ -113,9 +113,10 @@ public void testEquals() {
     public void testCarrierMethodType() {
         FunctionDescriptor fd = FunctionDescriptor.of(C_INT,
                 C_INT,
-                MemoryLayout.structLayout(C_INT, C_INT));
+                MemoryLayout.structLayout(C_INT, C_INT),
+                MemoryLayout.sequenceLayout(3, C_INT));
         MethodType cmt = fd.toMethodType();
-        assertEquals(cmt, MethodType.methodType(int.class, int.class, MemorySegment.class));
+        assertEquals(cmt, MethodType.methodType(int.class, int.class, MemorySegment.class, MemorySegment.class));
     }
 
     @Test(expectedExceptions = IllegalArgumentException.class)
@@ -140,4 +141,29 @@ public void testIllegalInsertArgOutOfBounds() {
         fd.insertArgumentLayouts(2, C_INT);
     }
 
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testBadPaddingInVoidFunction() {
+        FunctionDescriptor.ofVoid(MemoryLayout.paddingLayout(8));
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testBadPaddingInNonVoidFunction() {
+        FunctionDescriptor.of(MemoryLayout.paddingLayout(8));
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testBadPaddingInAppendArgLayouts() {
+        FunctionDescriptor.ofVoid().appendArgumentLayouts(MemoryLayout.paddingLayout(8));
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testBadPaddingInInsertArgLayouts() {
+        FunctionDescriptor.ofVoid().insertArgumentLayouts(0, MemoryLayout.paddingLayout(8));
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    public void testBadPaddingInChangeRetLayout() {
+        FunctionDescriptor.ofVoid().changeReturnLayout(MemoryLayout.paddingLayout(8));
+    }
+
 }

test/jdk/java/foreign/TestIllegalLink.java

@@ -109,14 +109,6 @@ public static Object[][] downcallOnlyOptions() {
     @DataProvider
     public static Object[][] types() {
         List<Object[]> cases = new ArrayList<>(Arrays.asList(new Object[][]{
-            {
-                    FunctionDescriptor.of(MemoryLayout.paddingLayout(64)),
-                    "Unsupported layout: x64"
-            },
-            {
-                    FunctionDescriptor.ofVoid(MemoryLayout.paddingLayout(64)),
-                    "Unsupported layout: x64"
-            },
             {
                     FunctionDescriptor.of(MemoryLayout.sequenceLayout(2, C_INT)),
                     "Unsupported layout: [2:i32]"