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

[Jorn Vernee]

On Mon, 4 Nov 2024 08:11:27 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 17 additional commits since the last revision:
> 
>  - Rephrase liker arg requirements
>  - Merge branch 'master' into linker-padding-layout-only
>  - Add rule checkings in AbstractLinker and add tests
>  - Merge branch 'master' into linker-padding-layout-only
>  - Update Linker docs
>  - Merge branch 'master' into linker-padding-layout-only
>  - Reword doce
>  - Add to specification and refine detection of PL GLs
>  - Add specific message for group layouts with only padding layouts
>  - Merge branch 'master' into linker-padding-layout-only
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/7d8aec13...0f4e93ff

src/java.base/share/classes/java/lang/foreign/Linker.java line 245:

> 243:  * <p>
> 244:  * A native linker only supports function descriptors whose argument/return layouts are
> 245:  * <em>well-formed</em> layouts. More formally, a layout `L`is well-formed if:

Suggestion:

 * <em>well-formed</em> layouts. More formally, a layout `L` is well-formed if:

src/java.base/share/classes/java/lang/foreign/Linker.java line 271:

> 269:  * </ul>
> 270:  * <p>
> 271:  * Well-formed layouts in function descriptions consumed by a native linker constitutes

Suggestion:

 * Well-formed layouts in function descriptions consumed by a native linker constitute

src/java.base/share/classes/java/lang/foreign/Linker.java line 278:

> 276:  * member layouts, as well as avoiding padding at the end of the struct layout.
> 277:  * For example:
> 278:   * {@snippet lang = java:

Pre-existing, but, AFAIK "allows to avoid" only works when there is an object between 'allows' and 'to', e.g. "allows _a client_ to avoid".

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

> 222: 
> 223:     // check elements are not all padding layouts and for trailing padding
> 224:     static private void checkGroup(GroupLayout gl, long maxUnpaddedOffset) {

Suggestion:

    private static void checkGroup(GroupLayout gl, long maxUnpaddedOffset) {

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

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

Suggestion:

    private static void assertIsAlignedBy(GroupLayout gl, long index, PaddingLayout padding, MemoryLayout element) {

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

> 268:                     " does not align the element " + element +
> 269:                     " (at byte offset " + pos + ")" + inMessage(gl));
> 270:         }

What happens here if an element is already aligned, and `padding.byteSize() == element.byteAlignment()`? e.g. `MemoryLayout.structLayout(MemoryLayout.paddingLayout(4), ValueLayout.JAVA_INT)`?

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

> 271:     }
> 272: 
> 273:     static String inMessage(GroupLayout gl) {

Suggestion:

    private static String inMessage(GroupLayout gl) {

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

> 280:     // checks both that there is no excess padding between 'memberLayout' and
> 281:     // the previous layout
> 282:     static private void checkMemberOffset(StructLayout parent, MemoryLayout memberLayout,

Suggestion:

    private static void checkMemberOffset(StructLayout parent, MemoryLayout memberLayout,

test/jdk/java/foreign/TestLinker.java line 183:

> 181: 
> 182:         var fd = FunctionDescriptor.of(struct, struct, struct);
> 183:         assertThrows(IllegalArgumentException.class, () ->linker.downcallHandle(fd));

Suggestion:

        assertThrows(IllegalArgumentException.class, () -> linker.downcallHandle(fd));

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827569106
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827585098
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827646537
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827630470
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827632076
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827636857
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827637004
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827639797
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827643127