8303002: Reject packed structs from linker

8300784: Specify exactly how padding should be presented to the linker
8304803: NPE thrown during downcall classification under Linux/x64
8303524: Check FunctionDescriptor byte order when linking

Reviewed-by: mcimadamore

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

@@ -34,6 +34,7 @@ import jdk.internal.reflect.CallerSensitive;
 import jdk.internal.reflect.Reflection;
 
 import java.lang.invoke.MethodHandle;
+import java.nio.ByteOrder;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -195,6 +196,17 @@ import java.util.stream.Stream;
  *     <td style="text-align:center;">{@link MemorySegment}</td>
  * </tbody>
  * </table></blockquote>
+ * <p>
+ * All the native linker implementations limit the function descriptors that they support to those that contain
+ * only so-called <em>canonical</em> layouts. A canonical layout has the following characteristics:
+ * <ol>
+ * <li>Its alignment constraint is set to its <a href="MemoryLayout.html#layout-align">natural alignment</a></li>
+ * <li>If it is a {@linkplain ValueLayout value layout}, its {@linkplain ValueLayout#order() byte order} is
+ * the {@linkplain ByteOrder#nativeOrder() native byte order}.
+ * <li>If it is a {@linkplain GroupLayout group layout}, its size is a multiple of its alignment constraint, and</li>
+ * <li>It does not contain padding other than what is strictly required to align its non-padding layout elements,
+ * or to satisfy constraint 3</li>
+ * </ol>
  *
  * <h3 id="function-pointers">Function pointers</h3>
  *

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

@@ -25,6 +25,7 @@
 package jdk.internal.foreign.abi;
 
 import jdk.internal.foreign.SystemLookup;
+import jdk.internal.foreign.Utils;
 import jdk.internal.foreign.abi.aarch64.linux.LinuxAArch64Linker;
 import jdk.internal.foreign.abi.aarch64.macos.MacOsAArch64Linker;
 import jdk.internal.foreign.abi.aarch64.windows.WindowsAArch64Linker;
@@ -40,9 +41,14 @@ import java.lang.foreign.Arena;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.foreign.Linker;
 import java.lang.foreign.MemorySegment;
+import java.lang.foreign.PaddingLayout;
 import java.lang.foreign.SequenceLayout;
+import java.lang.foreign.StructLayout;
+import java.lang.foreign.UnionLayout;
+import java.lang.foreign.ValueLayout;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 import java.util.Objects;
 
 public abstract sealed class AbstractLinker implements Linker permits LinuxAArch64Linker, MacOsAArch64Linker,
@@ -62,7 +68,7 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
     public MethodHandle downcallHandle(FunctionDescriptor function, Option... options) {
         Objects.requireNonNull(function);
         Objects.requireNonNull(options);
-        checkHasNaturalAlignment(function);
+        checkLayouts(function);
         LinkerOptions optionSet = LinkerOptions.forDowncall(function, options);
 
         return DOWNCALL_CACHE.get(new LinkRequest(function, optionSet), linkRequest ->  {
@@ -80,7 +86,7 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
         Objects.requireNonNull(arena);
         Objects.requireNonNull(target);
         Objects.requireNonNull(function);
-        checkHasNaturalAlignment(function);
+        checkLayouts(function);
         SharedUtils.checkExceptions(target);
         LinkerOptions optionSet = LinkerOptions.forUpcall(function, options);
 
@@ -101,22 +107,64 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
         return SystemLookup.getInstance();
     }
 
-    // Current limitation of the implementation:
+    /** {@return byte order used by this linker} */
-    // We don't support packed structs on some platforms,
+    protected abstract ByteOrder linkerByteOrder();
-    // so reject them here explicitly
+
-    private static void checkHasNaturalAlignment(FunctionDescriptor descriptor) {
+    private void checkLayouts(FunctionDescriptor descriptor) {
-        descriptor.returnLayout().ifPresent(AbstractLinker::checkHasNaturalAlignmentRecursive);
+        descriptor.returnLayout().ifPresent(this::checkLayoutsRecursive);
-        descriptor.argumentLayouts().forEach(AbstractLinker::checkHasNaturalAlignmentRecursive);
+        descriptor.argumentLayouts().forEach(this::checkLayoutsRecursive);
     }
 
-    private static void checkHasNaturalAlignmentRecursive(MemoryLayout layout) {
+    private void checkLayoutsRecursive(MemoryLayout layout) {
         checkHasNaturalAlignment(layout);
-        if (layout instanceof GroupLayout gl) {
+        if (layout instanceof ValueLayout vl) {
-            for (MemoryLayout member : gl.memberLayouts()) {
+            checkByteOrder(vl);
-                checkHasNaturalAlignmentRecursive(member);
+        } else if (layout instanceof StructLayout sl) {
+            long offset = 0;
+            long lastUnpaddedOffset = 0;
+            for (MemoryLayout member : sl.memberLayouts()) {
+                // check element offset before recursing so that an error points at the
+                // outermost layout first
+                checkMemberOffset(sl, member, lastUnpaddedOffset, offset);
+                checkLayoutsRecursive(member);
+
+                offset += member.bitSize();
+                if (!(member instanceof PaddingLayout)) {
+                    lastUnpaddedOffset = offset;
                 }
+            }
+            checkGroupSize(sl, lastUnpaddedOffset);
+        } else if (layout instanceof UnionLayout ul) {
+            long maxUnpaddedLayout = 0;
+            for (MemoryLayout member : ul.memberLayouts()) {
+                checkLayoutsRecursive(member);
+                if (!(member instanceof PaddingLayout)) {
+                    maxUnpaddedLayout = Long.max(maxUnpaddedLayout, member.bitSize());
+                }
+            }
+            checkGroupSize(ul, maxUnpaddedLayout);
         } else if (layout instanceof SequenceLayout sl) {
-            checkHasNaturalAlignmentRecursive(sl.elementLayout());
+            checkLayoutsRecursive(sl.elementLayout());
+        }
+    }
+
+    // check for trailing padding
+    private static void checkGroupSize(GroupLayout gl, long maxUnpaddedOffset) {
+        long expectedSize = Utils.alignUp(maxUnpaddedOffset, gl.bitAlignment());
+        if (gl.bitSize() != expectedSize) {
+            throw new IllegalArgumentException("Layout '" + gl + "' has unexpected size: "
+                    + gl.bitSize() + " != " + expectedSize);
+        }
+    }
+
+    // checks both that there is no excess padding between 'memberLayout' and
+    // the previous layout
+    private static void checkMemberOffset(StructLayout parent, MemoryLayout memberLayout,
+                                          long lastUnpaddedOffset, long offset) {
+        long expectedOffset = Utils.alignUp(lastUnpaddedOffset, memberLayout.bitAlignment());
+        if (expectedOffset != offset) {
+            throw new IllegalArgumentException("Member layout '" + memberLayout + "', of '" + parent + "'" +
+                    " found at unexpected offset: " + offset + " != " + expectedOffset);
         }
     }
 
@@ -125,4 +173,10 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
             throw new IllegalArgumentException("Layout bit alignment must be natural alignment: " + layout);
         }
     }
+
+    private void checkByteOrder(ValueLayout vl) {
+        if (vl.order() != linkerByteOrder()) {
+            throw new IllegalArgumentException("Layout does not have the right byte order: " + vl);
+        }
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64Linker.java

@@ -32,6 +32,7 @@ import jdk.internal.foreign.abi.aarch64.CallArranger;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 /**
  * ABI implementation based on ARM document "Procedure Call Standard for
@@ -60,4 +61,9 @@ public final class LinuxAArch64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.LINUX.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64Linker.java

@@ -32,6 +32,7 @@ import jdk.internal.foreign.abi.aarch64.CallArranger;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 /**
  * ABI implementation for macOS on Apple silicon. Based on AAPCS with
@@ -60,4 +61,9 @@ public final class MacOsAArch64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.MACOS.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64Linker.java

@@ -33,6 +33,7 @@ import jdk.internal.foreign.abi.aarch64.CallArranger;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 /**
  * ABI implementation for Windows/AArch64. Based on AAPCS with
@@ -57,4 +58,9 @@ public final class WindowsAArch64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.WINDOWS.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/fallback/FallbackLinker.java

@@ -43,6 +43,7 @@ import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
 import java.lang.ref.Reference;
+import java.nio.ByteOrder;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.function.Consumer;
@@ -117,6 +118,11 @@ public final class FallbackLinker extends AbstractLinker {
         };
     }
 
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.nativeOrder();
+    }
+
     private static MemorySegment makeCif(MethodType methodType, FunctionDescriptor function, FFIABI abi, Arena scope) {
         MemorySegment argTypes = scope.allocate(function.argumentLayouts().size() * ADDRESS.byteSize());
         List<MemoryLayout> argLayouts = function.argumentLayouts();

src/java.base/share/classes/jdk/internal/foreign/abi/riscv64/linux/LinuxRISCV64Linker.java

@@ -32,6 +32,7 @@ import jdk.internal.foreign.abi.LinkerOptions;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 public final class LinuxRISCV64Linker extends AbstractLinker {
 
@@ -56,4 +57,9 @@ public final class LinuxRISCV64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return LinuxRISCV64CallArranger.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVx64Linker.java

@@ -31,6 +31,7 @@ import jdk.internal.foreign.abi.LinkerOptions;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 /**
  * ABI implementation based on System V ABI AMD64 supplement v.0.99.6
@@ -58,4 +59,9 @@ public final class SysVx64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/Windowsx64Linker.java

@@ -30,6 +30,7 @@ import jdk.internal.foreign.abi.LinkerOptions;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
+import java.nio.ByteOrder;
 
 /**
  * ABI implementation based on Windows ABI AMD64 supplement v.0.99.6
@@ -57,4 +58,9 @@ public final class Windowsx64Linker extends AbstractLinker {
     protected UpcallStubFactory arrangeUpcall(MethodType targetType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.arrangeUpcall(targetType, function, options);
     }
+
+    @Override
+    protected ByteOrder linkerByteOrder() {
+        return ByteOrder.LITTLE_ENDIAN;
+    }
 }

test/jdk/java/foreign/TestIllegalLink.java

@@ -59,7 +59,7 @@ public class TestIllegalLink extends NativeTestHelper {
             fail("Expected IllegalArgumentException was not thrown");
         } catch (IllegalArgumentException e) {
             assertTrue(e.getMessage().contains(expectedExceptionMessage),
-                    e.getMessage() + " != " + expectedExceptionMessage);
+                    e.getMessage() + " does not contain " + expectedExceptionMessage);
         }
     }
 
@@ -154,7 +154,44 @@ public class TestIllegalLink extends NativeTestHelper {
                             ))),
                     "Layout bit alignment must be natural alignment"
             },
+            {
+                    FunctionDescriptor.ofVoid(MemoryLayout.structLayout(
+                            ValueLayout.JAVA_INT,
+                            MemoryLayout.paddingLayout(32), // no excess padding
+                            ValueLayout.JAVA_INT)),
+                    "unexpected offset"
+            },
+            {
+                    FunctionDescriptor.of(C_INT.withOrder(nonNativeOrder())),
+                    "Layout does not have the right byte order"
+            },
+            {
+                    FunctionDescriptor.of(MemoryLayout.structLayout(C_INT.withOrder(nonNativeOrder()))),
+                    "Layout does not have the right byte order"
+            },
+            {
+                    FunctionDescriptor.of(MemoryLayout.structLayout(MemoryLayout.sequenceLayout(C_INT.withOrder(nonNativeOrder())))),
+                    "Layout does not have the right byte order"
+            },
+            {
+                    FunctionDescriptor.ofVoid(MemoryLayout.structLayout(
+                            ValueLayout.JAVA_LONG,
+                            ValueLayout.JAVA_INT)), // missing trailing padding
+                    "has unexpected size"
+            },
+            {
+                    FunctionDescriptor.ofVoid(MemoryLayout.structLayout(
+                            ValueLayout.JAVA_INT,
+                            MemoryLayout.paddingLayout(32))), // too much trailing padding
+                    "has unexpected size"
+            },
         };
     }
 
+    private static ByteOrder nonNativeOrder() {
+        return ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN
+                ? ByteOrder.BIG_ENDIAN
+                : ByteOrder.LITTLE_ENDIAN;
+    }
+
 }

test/jdk/java/foreign/TestUpcallStructScope.java

@@ -58,7 +58,8 @@ public class TestUpcallStructScope extends NativeTestHelper {
     static final MemoryLayout S_PDI_LAYOUT = MemoryLayout.structLayout(
         C_POINTER.withName("p0"),
         C_DOUBLE.withName("p1"),
-        C_INT.withName("p2")
+        C_INT.withName("p2"),
+        MemoryLayout.paddingLayout(32)
     );
 
     static {