RFR: 8340205: Native linker allows MemoryLayout consisting of only PaddingLayout [v13]

[Jorn Vernee]

On Wed, 6 Nov 2024 14:18:50 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> This PR prevents sequence layout with padding to be used with the Linker.
>
> Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 30 additional commits since the last revision:
> 
>  - Simplify exception testing
>  - Merge branch 'master' into linker-padding-layout-only
>  - Remove redundant check
>  - Rephrase doc
>  - Improve language
>  - Add checks of exception messages
>  - Update test/jdk/java/foreign/TestLinker.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - ... and 20 more: https://git.openjdk.org/jdk/compare/3c99454c...6a97fe5d

I think some of the code could be simplified further, left some inline comments.

I also noticed that some cases involving unions are not handled as expected. For example:


MemoryLayout.unionLayout(MemoryLayout.paddingLayout(4), ValueLayout.JAVA_INT);
MemoryLayout.unionLayout(MemoryLayout.paddingLayout(2), ValueLayout.JAVA_INT);


Are both accepted, but I don't think they should be (they contains superfluous padding after all).

For the case:


MemoryLayout.unionLayout(MemoryLayout.paddingLayout(5), ValueLayout.JAVA_INT):


The exception message is: `The padding layout x5 is not of minimum size to align i4(with byte alignment 4) in [x5|i4]`, which is also a bit surprising, since in the case of unions, paddings are not used to align members, but only the union itself.

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

> 240:                 }
> 241:             }
> 242:         }

We are already looping over the members in `checkLayoutRecursive` I suggest adding to that loop instead of adding another one here.

Alignment of members is already handled by `checkMemberOffset` (the new loop also checks union members, which seems incorrect), so the only new thing that is needed from here is the check for consecutive padding layouts. It should be possible to handle that by keeping track of the previous layout, and then checking against the previous layout when we encounter a padding layout, inside the loop in `checkLayoutRecursive`.

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

> 245:                 gl.memberLayouts().getLast() instanceof PaddingLayout pl) {
> 246:             assertIsAlignedBy(null, 0, pl, gl);
> 247:         }

This was already being checked by the existing size check (currently at the end of this method). I don't think we need both checks?

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

> 257:     }
> 258: 
> 259:     private static void assertIsAlignedBy(GroupLayout gl, long index, PaddingLayout padding, MemoryLayout element) {

`index` is not used here.

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

PR Review: https://git.openjdk.org/jdk/pull/21041#pullrequestreview-2418470626
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1831144411
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1831137080
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1831155055