8299181: PaddingLayout unable to return byteAlignment value

Reviewed-by: mcimadamore

Author: minborg

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

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -36,6 +36,7 @@
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Stream;
+
 import jdk.internal.foreign.LayoutPath;
 import jdk.internal.foreign.LayoutPath.PathElementImpl.PathKind;
 import jdk.internal.foreign.Utils;
@@ -235,10 +236,7 @@ public sealed interface MemoryLayout permits SequenceLayout, GroupLayout, Paddin
      * @return the layout alignment constraint, in bytes.
      * @throws UnsupportedOperationException if {@code bitAlignment()} is not a multiple of 8.
      */
-    default long byteAlignment() {
-        return Utils.bitsToBytesOrThrow(bitAlignment(),
-                () -> new UnsupportedOperationException("Cannot compute byte alignment; bit alignment is not a multiple of 8"));
-    }
+    long byteAlignment();
 
     /**
      * Returns a memory layout of the same type with the same size and name as this layout,
@@ -611,15 +609,14 @@ static PathElement sequenceElement() {
     String toString();
 
     /**
-     * Creates a padding layout with the given size.
+     * Creates a padding layout with the given bitSize and a bit-alignment of eight.
      *
-     * @param size the padding size in bits.
+     * @param bitSize the padding size in bits.
      * @return the new selector layout.
-     * @throws IllegalArgumentException if {@code size <= 0}.
+     * @throws IllegalArgumentException if {@code bitSize < 0} or {@code bitSize % 8 != 0}
      */
-    static PaddingLayout paddingLayout(long size) {
-        MemoryLayoutUtil.checkSize(size);
-        return PaddingLayoutImpl.of(size);
+    static PaddingLayout paddingLayout(long bitSize) {
+        return PaddingLayoutImpl.of(MemoryLayoutUtil.requireBitSizeValid(bitSize));
     }
 
     /**
@@ -676,7 +673,7 @@ static ValueLayout valueLayout(Class<?> carrier, ByteOrder order) {
      * @throws IllegalArgumentException if {@code elementCount } is negative.
      */
     static SequenceLayout sequenceLayout(long elementCount, MemoryLayout elementLayout) {
-        MemoryLayoutUtil.checkSize(elementCount, true);
+        MemoryLayoutUtil.requireNonNegative(elementCount);
         Objects.requireNonNull(elementLayout);
         return wrapOverflow(() ->
                 SequenceLayoutImpl.of(elementCount, elementLayout));

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

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -49,6 +49,7 @@
  *
  * @implSpec implementing classes and subclasses are immutable, thread-safe and <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.
  *
+ * @sealedGraph
  * @since 19
  */
 @PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)

src/java.base/share/classes/jdk/internal/foreign/layout/AbstractGroupLayout.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -145,7 +145,7 @@ long alignof(List<MemoryLayout> elems) {
             return elems.stream()
                     .mapToLong(MemoryLayout::bitAlignment)
                     .max() // max alignment in case we have member layouts
-                    .orElse(1); // or minimal alignment if no member layout is given
+                    .orElse(8); // or minimal alignment if no member layout is given
         }
     }
 }

src/java.base/share/classes/jdk/internal/foreign/layout/AbstractLayout.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -25,10 +25,6 @@
  */
 package jdk.internal.foreign.layout;
 
-import jdk.internal.foreign.Utils;
-import jdk.internal.vm.annotation.ForceInline;
-import jdk.internal.vm.annotation.Stable;
-
 import java.lang.foreign.GroupLayout;
 import java.lang.foreign.MemoryLayout;
 import java.lang.foreign.SequenceLayout;
@@ -41,51 +37,47 @@
 public abstract sealed class AbstractLayout<L extends AbstractLayout<L> & MemoryLayout>
         permits AbstractGroupLayout, PaddingLayoutImpl, SequenceLayoutImpl, ValueLayouts.AbstractValueLayout {
 
-    private final long bitSize;
-    private final long bitAlignment;
+    private final long byteSize;
+    private final long byteAlignment;
     private final Optional<String> name;
-    @Stable
-    private long byteSize;
 
     AbstractLayout(long bitSize, long bitAlignment, Optional<String> name) {
-        this.bitSize = bitSize;
-        this.bitAlignment = bitAlignment;
-        this.name = name;
+        this.byteSize = MemoryLayoutUtil.requireBitSizeValid(bitSize) / 8;
+        this.byteAlignment = requirePowerOfTwoAndGreaterOrEqualToEight(bitAlignment) / 8;
+        this.name = Objects.requireNonNull(name);
     }
 
     public final L withName(String name) {
         Objects.requireNonNull(name);
-        return dup(bitAlignment, Optional.of(name));
+        return dup(bitAlignment(), Optional.of(name));
     }
 
     public final Optional<String> name() {
         return name;
     }
 
     public final L withBitAlignment(long bitAlignment) {
-        checkAlignment(bitAlignment);
         return dup(bitAlignment, name);
     }
 
     public final long bitAlignment() {
-        return bitAlignment;
+        return byteAlignment * 8;
+    }
+
+    public final long byteAlignment() {
+        return byteAlignment;
     }
 
-    @ForceInline
     public final long byteSize() {
-        if (byteSize == 0) {
-            byteSize = Utils.bitsToBytesOrThrow(bitSize(),
-                    () -> new UnsupportedOperationException("Cannot compute byte size; bit size is not a multiple of 8"));
-        }
         return byteSize;
     }
 
     public final long bitSize() {
-        return bitSize;
+        return byteSize * 8;
     }
 
     public boolean hasNaturalAlignment() {
-        return bitSize == bitAlignment;
+        return byteSize == byteAlignment;
     }
 
     // the following methods have to copy the same Javadoc as in MemoryLayout, or subclasses will just show
@@ -96,7 +88,7 @@ public boolean hasNaturalAlignment() {
      */
     @Override
     public int hashCode() {
-        return Objects.hash(name, bitSize, bitAlignment);
+        return Objects.hash(name, byteSize, byteAlignment);
     }
 
     /**
@@ -124,13 +116,14 @@ public boolean equals(Object other) {
 
         return other instanceof AbstractLayout<?> otherLayout &&
                 name.equals(otherLayout.name) &&
-                bitSize == otherLayout.bitSize &&
-                bitAlignment == otherLayout.bitAlignment;
+                byteSize == otherLayout.byteSize &&
+                byteAlignment == otherLayout.byteAlignment;
     }
 
     /**
      * {@return the string representation of this layout}
      */
+    @Override
     public abstract String toString();
 
     abstract L dup(long alignment, Optional<String> name);
@@ -140,17 +133,17 @@ String decorateLayoutString(String s) {
             s = String.format("%s(%s)", s, name().get());
         }
         if (!hasNaturalAlignment()) {
-            s = bitAlignment + "%" + s;
+            s = bitAlignment() + "%" + s;
         }
         return s;
     }
 
-    private static void checkAlignment(long alignmentBitCount) {
-        if (((alignmentBitCount & (alignmentBitCount - 1)) != 0L) || //alignment must be a power of two
-                (alignmentBitCount < 8)) { //alignment must be greater than 8
-            throw new IllegalArgumentException("Invalid alignment: " + alignmentBitCount);
+    private static long requirePowerOfTwoAndGreaterOrEqualToEight(long value) {
+        if (((value & (value - 1)) != 0L) || // value must be a power of two
+                (value < 8)) { // value must be greater or equal to 8
+            throw new IllegalArgumentException("Invalid alignment: " + value);
         }
+        return value;
     }
 
-
 }

src/java.base/share/classes/jdk/internal/foreign/layout/MemoryLayoutUtil.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -30,14 +30,18 @@ public final class MemoryLayoutUtil {
     private MemoryLayoutUtil() {
     }
 
-    public static void checkSize(long size) {
-        checkSize(size, false);
+    public static long requireNonNegative(long value) {
+        if (value < 0) {
+            throw new IllegalArgumentException("The provided value was negative: " + value);
+        }
+        return value;
     }
 
-    public static void checkSize(long size, boolean includeZero) {
-        if (size < 0 || (!includeZero && size == 0)) {
-            throw new IllegalArgumentException("Invalid size for layout: " + size);
+    public static long requireBitSizeValid(long bitSize) {
+        if (bitSize < 0 || bitSize % 8 != 0) {
+            throw new IllegalArgumentException("Invalid bitSize: " + bitSize);
         }
+        return bitSize;
     }
 
 }

src/java.base/share/classes/jdk/internal/foreign/layout/PaddingLayoutImpl.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -32,7 +32,7 @@
 public final class PaddingLayoutImpl extends AbstractLayout<PaddingLayoutImpl> implements PaddingLayout {
 
     private PaddingLayoutImpl(long bitSize) {
-        this(bitSize, 1, Optional.empty());
+        this(bitSize, 8, Optional.empty());
     }
 
     private PaddingLayoutImpl(long bitSize, long bitAlignment, Optional<String> name) {

src/java.base/share/classes/jdk/internal/foreign/layout/SequenceLayoutImpl.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ *  Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -68,7 +68,6 @@ public long elementCount() {
      * @throws IllegalArgumentException if {@code elementCount < 0}.
      */
     public SequenceLayout withElementCount(long elementCount) {
-        MemoryLayoutUtil.checkSize(elementCount, true);
         return new SequenceLayoutImpl(elementCount, elementLayout, bitAlignment(), name());
     }
 

test/jdk/java/foreign/TestLayoutPaths.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  This code is free software; you can redistribute it and/or modify it
@@ -168,25 +168,6 @@ public void testByteOffsetHandleBadRange() {
         seq.byteOffsetHandle(sequenceElement(0, 1)); // ranges not accepted
     }
 
-    @Test(expectedExceptions = UnsupportedOperationException.class)
-    public void testBadMultiple() {
-        GroupLayout g = MemoryLayout.structLayout(MemoryLayout.paddingLayout(3), JAVA_INT.withName("foo"));
-        g.byteOffset(groupElement("foo"));
-    }
-
-    @Test(expectedExceptions = UnsupportedOperationException.class)
-    public void testBadByteOffsetNoMultipleOf8() {
-        MemoryLayout layout = MemoryLayout.structLayout(MemoryLayout.paddingLayout(7), JAVA_INT.withName("x"));
-        layout.byteOffset(groupElement("x"));
-    }
-
-    @Test(expectedExceptions = UnsupportedOperationException.class)
-    public void testBadByteOffsetHandleNoMultipleOf8() throws Throwable {
-        MemoryLayout layout = MemoryLayout.structLayout(MemoryLayout.paddingLayout(7), JAVA_INT.withName("x"));
-        MethodHandle handle = layout.byteOffsetHandle(groupElement("x"));
-        handle.invoke();
-    }
-
     @Test
     public void testBadContainerAlign() {
         GroupLayout g = MemoryLayout.structLayout(JAVA_INT.withBitAlignment(16).withName("foo")).withBitAlignment(8);
@@ -364,10 +345,10 @@ public static Object[][] testLayouts() {
             (JAVA_INT.bitSize() * 2) * 4 + JAVA_INT.bitSize()
         });
         testCases.add(new Object[] {
-            MemoryLayout.sequenceLayout(10, MemoryLayout.structLayout(MemoryLayout.paddingLayout(5), JAVA_INT.withName("y"))),
+            MemoryLayout.sequenceLayout(10, MemoryLayout.structLayout(MemoryLayout.paddingLayout(8), JAVA_INT.withName("y"))),
             new PathElement[] { sequenceElement(), groupElement("y") },
             new long[] { 4 },
-            (JAVA_INT.bitSize() + 5) * 4 + 5
+            (JAVA_INT.bitSize() + 8) * 4 + 8
         });
         testCases.add(new Object[] {
             MemoryLayout.sequenceLayout(10, JAVA_INT),
@@ -441,45 +422,5 @@ public void testSliceHandle(MemoryLayout layout, PathElement[] pathElements, lon
         }
     }
 
-    @Test(expectedExceptions = UnsupportedOperationException.class)
-    public void testSliceHandleUOEInvalidOffsetEager() throws Throwable {
-        MemoryLayout layout = MemoryLayout.structLayout(
-            MemoryLayout.paddingLayout(5),
-            JAVA_INT.withName("y") // offset not a multiple of 8
-        );
-
-        layout.sliceHandle(groupElement("y")); // should throw
-    }
-
-    @Test(expectedExceptions = UnsupportedOperationException.class)
-    public void testSliceHandleUOEInvalidOffsetLate() throws Throwable {
-        MemoryLayout layout = MemoryLayout.sequenceLayout(3,
-            MemoryLayout.structLayout(
-                MemoryLayout.paddingLayout(4),
-                    JAVA_INT.withName("y") // offset not a multiple of 8
-            )
-        );
-
-        MethodHandle sliceHandle;
-        try {
-            sliceHandle = layout.sliceHandle(sequenceElement(), groupElement("y")); // should work
-        } catch (UnsupportedOperationException uoe) {
-            fail("Unexpected exception", uoe);
-            return;
-        }
-
-        try (Arena arena = Arena.openConfined()) {
-            MemorySegment segment = MemorySegment.allocateNative(layout, arena.scope());
-
-            try {
-                sliceHandle.invokeExact(segment, 1); // should work
-            } catch (UnsupportedOperationException uoe) {
-                fail("Unexpected exception", uoe);
-                return;
-            }
-
-            sliceHandle.invokeExact(segment, 0); // should throw
-        }
-    }
 }