On Wed, 20 Nov 2024 13:46:49 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 36 additional > commits since the last revision: > > - Update after comments > - Merge branch 'master' into linker-padding-layout-only > - Fix failing test > - Merge branch 'master' into linker-padding-layout-only > - Update after comments > - Merge branch 'master' into linker-padding-layout-only > - Simplify exception testing > - Merge branch 'master' into linker-padding-layout-only > - Remove redundant check > - Rephrase doc > - ... and 26 more: https://git.openjdk.org/jdk/compare/2f4888ed...28b3ad6d Could you also add test cases for the other two unions I mentioned? MemoryLayout.unionLayout(MemoryLayout.paddingLayout(4), ValueLayout.JAVA_INT); MemoryLayout.unionLayout(MemoryLayout.paddingLayout(5), ValueLayout.JAVA_INT): Both of those should be rejected. src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 222: > 220: if (!(member instanceof PaddingLayout pl)) { > 221: maxUnpaddedLayout = Long.max(maxUnpaddedLayout, > member.byteSize()); > 222: } else { I think we're still missing a check here to see if a union contains at most 1 padding layout? Something like this: MemoryLayout.unionLayout( MemoryLayout.sequenceLayout(3, ValueLayout.JAVA_INT), ValueLayout.JAVA_LONG, MemoryLayout.paddingLayout(16), MemoryLayout.paddingLayout(16)): Should be rejected. ------------- PR Review: https://git.openjdk.org/jdk/pull/21041#pullrequestreview-2449463637 PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1850835087
