8340205: Native linker allows MemoryLayout consisting of only PaddingLayout

Reviewed-by: jvernee, mcimadamore

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

@@ -241,50 +241,40 @@ import java.util.stream.Stream;
  * </tbody>
  * </table></blockquote>
  * <p>
- * All native linker implementations support a well-defined subset of layouts. More formally,
+ * A native linker only supports function descriptors whose argument/return layouts are
- * a layout {@code L} is supported by a native linker {@code NL} if:
+ * <em>well-formed</em> layouts. More formally, a layout `L` is well-formed if:
  * <ul>
- * <li>{@code L} is a value layout {@code V} and {@code V.withoutName()} is a canonical layout</li>
+ * <li>{@code L} is a value layout and {@code L} is derived from a canonical layout
+ *     {@code C} such that {@code L.byteAlignment() <= C.byteAlignment()}</li>
  * <li>{@code L} is a sequence layout {@code S} and all the following conditions hold:
  * <ol>
- * <li>the alignment constraint of {@code S} is set to its
+ * <li>{@code L.byteAlignment()} is equal to the sequence layout's <em>natural alignment</em>
- *     <a href="MemoryLayout.html#layout-align">natural alignment</a>, and</li>
+ *     , and</li>
- * <li>{@code S.elementLayout()} is a layout supported by {@code NL}.</li>
+ * <li>{@code S.elementLayout()} is a well-formed layout.</li>
  * </ol>
  * </li>
  * <li>{@code L} is a group layout {@code G} and all the following conditions hold:
  * <ol>
- * <li>the alignment constraint of {@code G} is set to its
+ * <li>{@code G.byteAlignment()} is equal to the group layout's <em>natural alignment</em></li>
- *     <a href="MemoryLayout.html#layout-align">natural alignment</a>;</li>
+ * <li>{@code G.byteSize()} is a multiple of {@code G.byteAlignment()}</li>
- * <li>the size of {@code G} is a multiple of its alignment constraint;</li>
+ * <li>Each member layout in {@code G.memberLayouts()} is either a padding layout or a
- * <li>each member layout in {@code G.memberLayouts()} is either a padding layout or
+ *     well-formed layout</li>
- *     a layout supported by {@code NL}, and</li>
+ * <li>Each non-padding member layout {@code E} in {@code G.memberLayouts()} follows an
- * <li>{@code G} does not contain padding other than what is strictly required to align
+ *     optional padding member layout, whose size is the minimum size required to
- *      its non-padding layout elements, or to satisfy (2).</li>
+ *     align {@code E}</li>
+ * <li>{@code G} contains an optional trailing padding member layout, whose size is the
+ *     minimum size that satisfies (2)</li>
  * </ol>
  * </li>
  * </ul>
- *
- * Linker implementations may optionally support additional layouts, such as
- * <em>packed</em> struct layouts. A packed struct is a struct in which there is
- * at least one member layout {@code L} that has an alignment constraint less strict
- * than its natural alignment. This allows to avoid padding between member layouts,
- * as well as avoiding padding at the end of the struct layout. For example:
-
- * {@snippet lang = java:
- * // No padding between the 2 element layouts:
- * MemoryLayout noFieldPadding = MemoryLayout.structLayout(
- *         ValueLayout.JAVA_INT,
- *         ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
- *
- * // No padding at the end of the struct:
- * MemoryLayout noTrailingPadding = MemoryLayout.structLayout(
- *         ValueLayout.JAVA_DOUBLE.withByteAlignment(4),
- *         ValueLayout.JAVA_INT);
- * }
  * <p>
- * A native linker only supports function descriptors whose argument/return layouts are
+ * A function descriptor is well-formed if its argument and return layouts are
- * layouts supported by that linker and are not sequence layouts.
+ * well-formed and are not sequence layouts. A native linker is guaranteed to reject
+ * function descriptors that are not well-formed. However, a native linker can still
+ * reject well-formed function descriptors, according to platform-specific rules.
+ * For example, some native linkers may reject <em>packed</em> struct layouts -- struct
+ * layouts whose member layouts feature relaxed alignment constraints, to avoid
+ * the insertion of additional padding.
  *
  * <h3 id="function-pointers">Function pointers</h3>
  *

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

@@ -57,7 +57,6 @@ import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
 import java.util.HashSet;
 import java.util.List;
-import java.nio.ByteOrder;
 import java.util.Objects;
 import java.util.Set;
 
@@ -189,6 +188,7 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
             checkHasNaturalAlignment(layout);
             long offset = 0;
             long lastUnpaddedOffset = 0;
+            PaddingLayout preceedingPadding = null;
             for (MemoryLayout member : sl.memberLayouts()) {
                 // check element offset before recursing so that an error points at the
                 // outermost layout first
@@ -196,29 +196,65 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
                 checkStructMember(member, offset);
 
                 offset += member.byteSize();
-                if (!(member instanceof PaddingLayout)) {
+                if (!(member instanceof PaddingLayout pl)) {
                     lastUnpaddedOffset = offset;
+                    if (preceedingPadding != null) {
+                        preceedingPadding = null;
+                    }
+                } else {
+                    if (preceedingPadding != null) {
+                        throw new IllegalArgumentException("The padding layout " + pl +
+                                " was preceded by another padding layout " + preceedingPadding +
+                                inMessage(sl));
+                    }
+                    preceedingPadding = pl;
                 }
             }
-            checkGroupSize(sl, lastUnpaddedOffset);
+            checkNotAllPadding(sl);
+            checkGroup(sl, lastUnpaddedOffset);
         } else if (layout instanceof UnionLayout ul) {
             checkHasNaturalAlignment(layout);
-            long maxUnpaddedLayout = 0;
+            // We need to know this up front
+            long maxUnpaddedLayout = ul.memberLayouts().stream()
+                    .filter(l -> !(l instanceof PaddingLayout))
+                    .mapToLong(MemoryLayout::byteSize)
+                    .max()
+                    .orElse(0);
+
+            boolean hasPadding = false;
+
             for (MemoryLayout member : ul.memberLayouts()) {
                 checkLayoutRecursive(member);
-                if (!(member instanceof PaddingLayout)) {
+                if (member instanceof PaddingLayout pl) {
-                    maxUnpaddedLayout = Long.max(maxUnpaddedLayout, member.byteSize());
+                    if (hasPadding) {
+                        throw new IllegalArgumentException("More than one padding" + inMessage(ul));
+                    }
+                    hasPadding = true;
+                    if (pl.byteSize() <= maxUnpaddedLayout) {
+                        throw new IllegalArgumentException("Superfluous padding " + pl + inMessage(ul));
+                    }
                 }
             }
-            checkGroupSize(ul, maxUnpaddedLayout);
+            checkGroup(ul, maxUnpaddedLayout);
         } else if (layout instanceof SequenceLayout sl) {
             checkHasNaturalAlignment(layout);
+            if (sl.elementLayout() instanceof PaddingLayout pl) {
+                throw memberException(sl, pl,
+                        "not supported because a sequence of a padding layout is not allowed");
+            }
             checkLayoutRecursive(sl.elementLayout());
         }
     }
 
-    // check for trailing padding
+    // check elements are not all padding layouts
-    private void checkGroupSize(GroupLayout gl, long maxUnpaddedOffset) {
+    private static void checkNotAllPadding(StructLayout sl) {
+        if (!sl.memberLayouts().isEmpty() && sl.memberLayouts().stream().allMatch(e -> e instanceof PaddingLayout)) {
+            throw new IllegalArgumentException("Layout '" + sl + "' is non-empty and only has padding layouts");
+        }
+    }
+
+    // check trailing padding
+    private static void checkGroup(GroupLayout gl, long maxUnpaddedOffset) {
         long expectedSize = Utils.alignUp(maxUnpaddedOffset, gl.byteAlignment());
         if (gl.byteSize() != expectedSize) {
             throw new IllegalArgumentException("Layout '" + gl + "' has unexpected size: "
@@ -226,17 +262,28 @@ public abstract sealed class AbstractLinker implements Linker permits LinuxAArch
         }
     }
 
+    private static String inMessage(GroupLayout gl) {
+        return " in " + gl;
+    }
+
     // checks both that there is no excess padding between 'memberLayout' and
     // the previous layout
-    private void checkMemberOffset(StructLayout parent, MemoryLayout memberLayout,
+    private static void checkMemberOffset(StructLayout parent, MemoryLayout memberLayout,
                                           long lastUnpaddedOffset, long offset) {
         long expectedOffset = Utils.alignUp(lastUnpaddedOffset, memberLayout.byteAlignment());
         if (expectedOffset != offset) {
-            throw new IllegalArgumentException("Member layout '" + memberLayout + "', of '" + parent + "'" +
+            throw memberException(parent, memberLayout,
-                    " found at unexpected offset: " + offset + " != " + expectedOffset);
+                    "found at unexpected offset: " + offset + " != " + expectedOffset);
         }
     }
 
+    private static IllegalArgumentException memberException(MemoryLayout parent,
+                                                            MemoryLayout member,
+                                                            String info) {
+        return new IllegalArgumentException(
+                "Member layout '" + member + "', of '" + parent + "' " + info);
+    }
+
     private void checkSupported(ValueLayout valueLayout) {
         valueLayout = valueLayout.withoutName();
         if (valueLayout instanceof AddressLayout addressLayout) {

test/jdk/java/foreign/TestLinker.java

@@ -35,6 +35,9 @@ import org.testng.annotations.Test;
 import java.lang.foreign.FunctionDescriptor;
 import java.lang.foreign.Linker;
 import java.lang.foreign.MemoryLayout;
+import java.lang.foreign.PaddingLayout;
+import java.lang.foreign.SequenceLayout;
+import java.lang.foreign.StructLayout;
 import java.lang.foreign.ValueLayout;
 import java.lang.invoke.MethodHandle;
 import java.util.ArrayList;
@@ -42,12 +45,8 @@ import java.util.Arrays;
 import java.util.List;
 
 import static java.lang.foreign.MemoryLayout.*;
-import static java.lang.foreign.ValueLayout.JAVA_CHAR;
+import static java.lang.foreign.ValueLayout.*;
-import static java.lang.foreign.ValueLayout.JAVA_SHORT;
+import static org.testng.Assert.*;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertSame;
-import static org.testng.Assert.assertNotSame;
-import static org.testng.Assert.assertTrue;
 
 public class TestLinker extends NativeTestHelper {
 
@@ -150,6 +149,110 @@ public class TestLinker extends NativeTestHelper {
         assertTrue(layout instanceof ValueLayout);
     }
 
+    @Test
+    public void embeddedPaddingLayout() {
+        PaddingLayout padding = MemoryLayout.paddingLayout(64).withByteAlignment(64);
+        SequenceLayout sequence = MemoryLayout.sequenceLayout(2, padding);
+        StructLayout struct = MemoryLayout.structLayout(sequence);
+        FunctionDescriptor fd = FunctionDescriptor.of(struct, struct);
+        Linker linker = Linker.nativeLinker();
+        var x = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertTrue(x.getMessage().contains("not supported because a sequence of a padding layout is not allowed"));
+    }
+
+    @Test
+    public void groupLayoutWithOnlyPadding() {
+        PaddingLayout padding = MemoryLayout.paddingLayout(1);
+        StructLayout struct = MemoryLayout.structLayout(padding);
+        FunctionDescriptor fd = FunctionDescriptor.of(struct, struct);
+        Linker linker = Linker.nativeLinker();
+        var x = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertTrue(x.getMessage().contains("is non-empty and only has padding layouts"));
+    }
+
+    @Test
+    public void interwovenPadding() {
+        Linker linker = Linker.nativeLinker();
+        var padding1 = MemoryLayout.paddingLayout(1);
+        var padding2 = MemoryLayout.paddingLayout(2).withByteAlignment(2);
+
+        var struct = MemoryLayout.structLayout(JAVA_BYTE, padding1, padding2, JAVA_INT);
+
+        var fd = FunctionDescriptor.of(struct, struct, struct);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(),
+                "The padding layout x2 was preceded by another padding layout x1 in [b1x1x2i4]");
+    }
+
+    @Test
+    public void stackedPadding() {
+        Linker linker = Linker.nativeLinker();
+        var struct32 = MemoryLayout.structLayout(MemoryLayout.sequenceLayout(4, JAVA_LONG));
+        var padding1 = MemoryLayout.paddingLayout(1);
+        var padding2 = MemoryLayout.paddingLayout(2).withByteAlignment(2);
+        var padding4 = MemoryLayout.paddingLayout(4).withByteAlignment(4);
+        var padding8 = MemoryLayout.paddingLayout(8).withByteAlignment(8);
+        var padding16 = MemoryLayout.paddingLayout(16).withByteAlignment(16);
+        var padding32 = MemoryLayout.paddingLayout(32).withByteAlignment(32);
+        var union = MemoryLayout.unionLayout(struct32, padding32);
+        var struct = MemoryLayout.structLayout(JAVA_BYTE, padding1, padding2, padding4, padding8, padding16, union);
+        var fd = FunctionDescriptor.of(struct, struct, struct);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(),
+                "The padding layout x2 was preceded by another padding layout x1 in [b1x1x2x4x8x16[[[4:j8]]|x32]]");
+    }
+
+    @Test
+    public void paddingUnionByteSize3() {
+        Linker linker = Linker.nativeLinker();
+        var union = MemoryLayout.unionLayout(MemoryLayout.paddingLayout(3), ValueLayout.JAVA_INT);
+        var fd = FunctionDescriptor.of(union, union, union);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(), "Superfluous padding x3 in [x3|i4]");
+    }
+
+    @Test
+    public void paddingUnionByteSize4() {
+        Linker linker = Linker.nativeLinker();
+        var union = MemoryLayout.unionLayout(MemoryLayout.paddingLayout(4), ValueLayout.JAVA_INT);
+        var fd = FunctionDescriptor.of(union, union, union);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(), "Superfluous padding x4 in [x4|i4]");
+    }
+
+    @Test
+    public void paddingUnionByteSize5() {
+        Linker linker = Linker.nativeLinker();
+        var union = MemoryLayout.unionLayout(MemoryLayout.paddingLayout(5), ValueLayout.JAVA_INT);
+        var fd = FunctionDescriptor.of(union, union, union);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(), "Layout '[x5|i4]' has unexpected size: 5 != 4");
+    }
+
+    @Test
+    public void paddingUnionSeveral() {
+        Linker linker = Linker.nativeLinker();
+        var union = MemoryLayout.unionLayout(
+                MemoryLayout.sequenceLayout(3, ValueLayout.JAVA_INT),
+                ValueLayout.JAVA_LONG,
+                MemoryLayout.paddingLayout(16),
+                MemoryLayout.paddingLayout(16));
+        var fd = FunctionDescriptor.of(union, union, union);
+        var e = expectThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));
+        assertEquals(e.getMessage(), "More than one padding in [[3:i4]|j8|x16|x16]");
+    }
+
+    @Test
+    public void sequenceOfZeroElements() {
+        Linker linker = Linker.nativeLinker();
+        var sequence0a8 = MemoryLayout.sequenceLayout(0, JAVA_LONG);
+        var sequence3a1 = MemoryLayout.sequenceLayout(3, JAVA_BYTE);
+        var padding5a1 = MemoryLayout.paddingLayout(5);
+        var struct8a8 = MemoryLayout.structLayout(sequence0a8, sequence3a1, padding5a1);
+        var fd = FunctionDescriptor.of(struct8a8, struct8a8, struct8a8);
+        linker.downcallHandle(fd);
+    }
+
     @DataProvider
     public static Object[][] canonicalTypeNames() {
         return new Object[][]{