RFR: 7903587: Revisit inlining of struct layouts into other layouts/descriptors [v2]

[Jorn Vernee]

On Tue, 12 Dec 2023 15:51:59 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR completes the sequence of changes that I've been working on, by allowing layout and descriptor strings to be generated directly from jextract types. This allows for generated layouts to respect the "structure" found in the corresponding C header. Consider this case:
>> 
>> 
>> struct Point {
>>    int x;
>>    int y;
>> };
>> 
>> struct Rectangle {
>>    struct Point topLeft;
>>    struct Point bottomRight;
>> };
>> 
>> 
>> With the changes in this PR, the generate layout for `Rectangle` becomes:
>> 
>> 
>> private static final MemoryLayout $LAYOUT = MemoryLayout.structLayout(
>>         Point.$LAYOUT().withName("topLeft"),
>>         Point.$LAYOUT().withName("bottomRight")
>>     ).withName("Rectangle");
>> 
>> 
>> Note how the layout for `Point` is no longer inlined, but instead is referred to "by name".
>> 
>> The patch is quite straightforward, and aims at replacing the functions Declaration/Type.layoutFor/descriptorFor with new functions (now in the builder hierarchy) which generate layout and descriptor strings from the corresponding jextract types.
>> 
>> Note: this patch introduces some additional issues when it comes with static initializer circularities. These issues will be dealt with in a separate change (since we have started accumulating some of these since https://git.openjdk.org/jextract/pull/145)
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add new packed struct test

src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java line 285:

> 283:     private String alignIfNeeded(String layoutPrefix, long align, long expectedAlign) {
> 284:         return align > expectedAlign ?
> 285:                 STR."\{layoutPrefix}.withByteAlignment(\{expectedAlign})" :

We never take the branch that adjusts the alignment here.

src/main/java/org/openjdk/jextract/impl/DeclarationImpl.java line 332:

> 330:             return firstDecl.isEmpty() ?
> 331:                     OptionalLong.empty() :
> 332:                     recordMemberOffset(firstDecl.get());

Pre-existing, but could you add a comment to this `else` block to say that we are handling an anonymous nested struct?

src/main/java/org/openjdk/jextract/impl/StructBuilder.java line 310:

> 308:                 if ((ClangAlignOf.getOrThrow(member) / 8) > align) {
> 309:                     memberLayout = STR."\{memberLayout}.withByteAlignment(\{align})";
> 310:                 }

Looking at `layoutString` it will already add calls to `withByteAlignment` if needed (through `alignIfNeeded`). So, I don't think it needs to be done here? But, I think `structOrUnionLayoutString` is missing a call for the whole struct/union.

src/main/java/org/openjdk/jextract/impl/StructBuilder.java line 325:

> 323:         if (size != expectedSize) {
> 324:             memberLayouts.add(paddingLayoutString((expectedSize - size) / 8));
> 325:         }

Also pre-exisiting, but this isn't right for unions. e.g. in a case like this:


union U {
    double d;
    int arr[3];
};


The total size of the fields is 12 bytes, but alignment is 8 bytes due to the `double`, so we need some alignment padding to make the size a multiple of 8.

i.e. I think for unions the padding added here should be `expectedSize / 8`.

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java line 69:

> 67:             public static final ValueLayout.OfShort C_SHORT = ValueLayout.JAVA_SHORT;
> 68:             public static final ValueLayout.OfInt C_INT = ValueLayout.JAVA_INT;
> 69:             public static final ValueLayout.OfLong C_LONG = ValueLayout.JAVA_\{longType};

For Windows this should be `ValueLayout.OfInt`.

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java line 77:

> 75:             """);
> 76:         if (TypeImpl.IS_WINDOWS) {
> 77:             first.appendIndentedLines("public static final AddressLayout C_LONG_DOUBLE = ValueLayout.JAVA_DOUBLE;");

Should be `ValueLayout.OfDouble`.

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

PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424165880
PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424169051
PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424211010
PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424192952
PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424140682
PR Review Comment: https://git.openjdk.org/jextract/pull/157#discussion_r1424140947