On Mon, 4 Nov 2024 12:13:26 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> 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/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)`? Also, don't we already check the alignment before when calling `checkMemberOffset`? Why is it checked again here? > 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)); Could you please check the exception message as well? And in `stackedPadding` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827639666 PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1827644509
