RFR: 8317545: AIX PPC64: Implementation of Foreign Function & Memory API [v2]

[Jorn Vernee]

On Sat, 14 Oct 2023 13:54:28 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> Thanks for your outstanding support! I'm awaiting feedback from IBM and a solution for the subtask https://bugs.openjdk.org/browse/JDK-8317799. There shouldn't be much work to be done after that, I guess.
>
> So, your proposal should look like this?
> 
> diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
> index dbd9a3f67a4..ec1639938e8 100644
> --- a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
> +++ b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
> @@ -180,6 +180,11 @@ private void checkLayout(MemoryLayout layout) {
>          }
>      }
>  
> +    // Platforms can override.
> +    protected MemoryLayout processGroupLayoutMember(MemoryLayout member, long offset) {
> +        return member;
> +    }
> +
>      private void checkLayoutRecursive(MemoryLayout layout) {
>          if (layout instanceof ValueLayout vl) {
>              checkSupported(vl);
> @@ -191,6 +196,7 @@ private void checkLayoutRecursive(MemoryLayout layout) {
>                  // check element offset before recursing so that an error points at the
>                  // outermost layout first
>                  checkMemberOffset(sl, member, lastUnpaddedOffset, offset);
> +                member = processGroupLayoutMember(member, offset);
>                  checkLayoutRecursive(member);
>  
>                  offset += member.byteSize();
> diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
> index c24d2553ec0..ddd4049c814 100644
> --- a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
> +++ b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
> @@ -46,7 +46,7 @@ public final class AixPPC64Linker extends AbstractLinker {
>      static {
>          HashMap<String, MemoryLayout> layouts = new HashMap<>();
>          layouts.putAll(SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT));
> -        layouts.put("double", ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
> +        layouts.put("double4bytealigned", ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
>          CANONICAL_LAYOUTS = Map.copyOf(layouts);
>      }
>  
> @@ -81,4 +81,13 @@ protected ByteOrder linkerByteOrder() {
>      public Map<String, MemoryLayout> canonicalLayouts() {
>          return CANONICAL_LAYOUTS;
>      }
> +
> +    @Override
> +    protected MemoryLayout processGroupLayoutMember(MemoryLayout member, long offset) {
> +        // Change alignment of double members to 4 except at the beginning.
> +        if ((offse...

I was thinking something like this:


diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
index dbd9a3f67a4..10b371457b3 100644
--- a/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
+++ b/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
@@ -180,6 +180,11 @@ private void checkLayout(MemoryLayout layout) {
         }
     }
 
+    // some ABIs have special handling for struct members
+    protected void checkStructMember(MemoryLayout member, long offset) {
+        checkLayoutRecursive(member);
+    }
+
     private void checkLayoutRecursive(MemoryLayout layout) {
         if (layout instanceof ValueLayout vl) {
             checkSupported(vl);
@@ -191,7 +196,7 @@ private void checkLayoutRecursive(MemoryLayout layout) {
                 // check element offset before recursing so that an error points at the
                 // outermost layout first
                 checkMemberOffset(sl, member, lastUnpaddedOffset, offset);
-                checkLayoutRecursive(member);
+                checkStructMember(member, offset);
 
                 offset += member.byteSize();
                 if (!(member instanceof PaddingLayout)) {
diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
index c24d2553ec0..f70af2bc025 100644
--- a/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
+++ b/src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/aix/AixPPC64Linker.java
@@ -36,19 +36,14 @@
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodType;
 import java.nio.ByteOrder;
-import java.util.HashMap;
 import java.util.Map;
 
 public final class AixPPC64Linker extends AbstractLinker {
 
-    static final Map<String, MemoryLayout> CANONICAL_LAYOUTS;
+    static final Map<String, MemoryLayout> CANONICAL_LAYOUTS
+            = SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT);
 
-    static {
-        HashMap<String, MemoryLayout> layouts = new HashMap<>();
-        layouts.putAll(SharedUtils.canonicalLayouts(ValueLayout.JAVA_LONG, ValueLayout.JAVA_LONG, ValueLayout.JAVA_INT));
-        layouts.put("double", ValueLayout.JAVA_DOUBLE.withByteAlignment(4));
-        CANONICAL_LAYOUTS = Map.copyOf(layouts);
-    }
+    private static final MemoryLayout C_DOUBLE = CANONICAL_LAYOUTS.get("double");
 
     public static AixPPC64Linker getInstance() {
         final class Holder {
@@ -62,6 +57,19 @@ private AixPPC64Linker() {
         // Ensure there is only one instance
     }
 
+    @Override
+    protected void checkStructMember(MemoryLayout member, long offset) {
+        // special case double members that are not the first member
+        // see: https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes
+        if ((offset > 0) && member.equals(C_DOUBLE)) {
+            if (member.byteAlignment() != 4) {
+                throw new IllegalArgumentException("double struct member following the first member should be 4-byte aligned");
+            }
+        } else {
+            super.checkStructMember(member, offset);
+        }
+    }
+
     @Override
     protected MethodHandle arrangeDowncall(MethodType inferredMethodType, FunctionDescriptor function, LinkerOptions options) {
         return CallArranger.AIX.arrangeDowncall(inferredMethodType, function, options);



I.e. there should not be a special 4-byte aligned canonical layout. Canonical layouts are for mapping native/C type names to memory layouts, and `double4bytealigned` is not such a name.

Then, with the above changes, you'd also need to change the tests to use the right double layouts when creating struct layouts on AIX. Note that that will also require changes to `jdk.internal.foreign.Utils::computePaddedStructLayout` which computes struct layouts for some of the tests (including all the `TestDowncall*` and `TestUpcall*` tests). In that method, there are a couple of calls to `l.byteAlignment()` that should be replaced by something that returns `4` for non-first double layouts on AIX.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16179#discussion_r1359480986