RFR: 8340205: Native linker allows MemoryLayout consisting of only PaddingLayout [v3]
Per Minborg
pminborg at openjdk.org
Mon Sep 23 10:53:38 UTC 2024
On Mon, 23 Sep 2024 10:19:43 GMT, Maurizio Cimadamore <mcimadamore at 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 nine additional commits since the last revision:
>>
>> - Add specific message for group layouts with only padding layouts
>> - Merge branch 'master' into linker-padding-layout-only
>> - Check exception message
>> - Remove redundant doc section
>> - Merge branch 'master' into linker-padding-layout-only
>> - Improve excception message
>> - Document a sequence layout cannot contain a padding layout
>> - Update copyright year
>> - Prevent embeded only padding layout in linker
>
> src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 225:
>
>> 223: // check elements are not all padding layouts and for trailing padding
>> 224: private void checkGroup(GroupLayout gl, long maxUnpaddedOffset) {
>> 225: if (gl.memberLayouts().stream().allMatch(e -> e instanceof PaddingLayout)) {
>
> I'm not 100% sure that this check (which I agree with) can be desumed from the current `Linker` javadoc. E.g. if I have:
>
> `groupLayout(paddingLayout(1))`
>
> The Linker says:
> * there cannot be any intermediate padding more than necessary to align non-padding elements. But there's no non-padding elements here, so <shrug> :-)
> * there cannot be more padding at the end, more than necessary to make the group size a multiple of its alignment - but again, since there's no non-padding layout, it is not clear what this means here. We have a padding of 1, which makes the group have size 1, which is a multiple of the alignment of the struct. So, ok?
Previously, the check threw an `IllegalArgumentException` when presented with the above (or other) group layout with only padding layouts, but the actual message differed. So, this change only changes the exception message and does not change the current behavior. However, the question remains: Should we update the docs to reflect this behavior?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21041#discussion_r1771187107
More information about the core-libs-dev
mailing list