On Mon, 4 Nov 2024 08:11:27 GMT, Per Minborg <pminb...@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
