[foreign-jextract] RFR: 8252759: Jextract flattens nested anonymous structs and unions when computing layouts

[Jorn Vernee]

On Fri, 15 Jan 2021 11:50:15 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch changes the way jextract handles nested structs and unions.
>> 
>> Currently, when encountering an anonymous struct or union, jextract folds it's fields into the parent layout. But, this can lead to incorrect layouts, especially for unions. e.g. see the following example:
>> 
>>     struct Foo {
>>         int x;
>>         union {
>>             int y;
>>             int z;
>>         };
>>     }; 
>>     
>> becomes:
>> 
>>     static final MemoryLayout Foo$struct$LAYOUT_ = MemoryLayout.ofStruct(
>>         C_INT.withName("x"),
>>         C_INT.withName("y"),
>>         C_INT.withName("z")
>>     ).withName("Foo"); 
>>     
>> In this case, the offset of the `z` field will not be correct.
>> 
>> This patch changes the jextract behaviour to preserve the nested layouts of anonymous unions and structs, so the example becomes:
>> 
>>     static final MemoryLayout Foo$struct$LAYOUT_ = MemoryLayout.ofStruct(
>>         C_INT.withName("x"),
>>         MemoryLayout.ofUnion(
>>             C_INT.withName("y"),
>>             C_INT.withName("z")
>>         ).withName("$anon$0")
>>     ).withName("Foo"); 
>>     
>> Here, the anonymous layout gets a generated name which is needed so that the `y` and `z` fields can still be reached from the root layout with layout paths to create var handles for instance.
>> 
>> Just the layout is changed, the Foo java class that we generate still has getters and setters for `y` and `z`.
>> 
>> In the patch this behaviour is implemented by temporarily adding a 'prefix' (`"$anon$0"` in this case) to the `StructBuilder`, which is used in the generated layout paths to reach the nested fields.
>> 
>> Thanks,
>> Jorn
>
> I like the approach, because not only it fixes code generation, but it also makes the AST "right" by avoiding early flattening. I think the test is a bit light - we might want to add more cases of deeply nested anon structs, just in case - as this area is a bit fragile.

Note that we already have a test that more broadly tests nested structs; https://github.com/openjdk/panama-foreign/blob/foreign-jextract/test/jdk/tools/jextract/nested.h

It just wasn't catching this particular case, so I added something separate. I can enhance that test to also check offsets of fields.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/434