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

[Jorn Vernee]

On Fri, 15 Jan 2021 11:48:52 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
>
> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/RecordLayoutComputer.java line 108:
> 
>> 106: 
>> 107:     void addFieldLayout(long offset, Type parent, Cursor c) {
>> 108:         MemoryLayout memoryLayout = c.isAnonymousStruct()
> 
> So, am I right that this avoid flattening of the anon structs fields into enclosing layout - e.g. layouts coming out of the parser now are non-flattened, but we tag them with an attribute so that we can emit flattened accessors later?

Yeah. The layout is only tagged if it is nested inside a parent record layout, since we can only assign a synthetic unique name to it in the context of the parent layout. But, the layout attached to the Declaration object for the anonymous struct will not have this name, since it is created separately outside of the context of the parent struct.

That means that when generating code, and when we counter an anonymous nested struct/union decl in visitScoped, we have to look up it's layout in the layout of the parent in order to find the context dependent name we attached here. The attribute is used to find anonymous layouts. Technically we could also check if the name starts with `$anon$` instead of looking for the attribute, but doing it this way seemed more robust.

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

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