On Sat, 14 Oct 2023 16:56:50 GMT, Jorn Vernee <jver...@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
