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

[Jorn Vernee]

On Sat, 14 Oct 2023 16:56:50 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> 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) {
>> + ...
>
> 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(layo...

Err, actually using `member.equals(C_DOUBLE)` in the above doesn't work since it still checks alignment. What you have with checking for `ValueLayout` and `carrier() == double.class` is better, but the byte order should also be checked at some point.

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

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