[foreign] RFR 8217380: LayoutType::ofStruct should try to resolve Struct layout

[Maurizio Cimadamore]

On 21/01/2019 11:21, Jorn Vernee wrote:
>> but imho Address should override isPartial() to always return `true`
>
> I mean `false` here. i.e. an Address is never partial, it is always a 
> complete type.

It depends on the point of view - right now the API consider partial any 
layout that has some unresolved layout in its guts.

But, as you observe, not all partial layouts are created equals - and 
there are two possible things you might want to do with them:

1) ask for size
2) dereference

If you just need (1), then excluding addresses from the isPartial 
business (as you suggest) is the right thing; but if you need (2), that 
is not enough.

Looking at how the code uses 'isPartial', especially in LayoutResolver, 
I think (1) might be the best semantics; note that if I have a partial 
layout according to the (2) definition, and I resolve it using a 
LayoutResolver, nothing will happen, as LayoutResolver will only affect 
Sequence, Group and Unresolved, leaving Address alone.

So I think changing isPartial to reflect that would lead to an 
improvement. We can also update the javadoc of Descriptor::isPartial to 
specify exactly what the semantics is.

Maurizio

>
> Jorn
>
> Jorn Vernee schreef op 2019-01-21 12:11:
>> Ok thanks,
>>
>> FWIW, I thought it was best to try to do the resolution as early as
>> possible to avoid having to do it multiple times later on. We need the
>> carrier type (which has the annotations) to do the resolution, and the
>> earliest time carrier and layout come together seems to be in
>> LayoutType.
>>
>> The other place where we could try and resolve the layout is in Scope,
>> right before the allocation, since that's when resolution is needed.
>> After sending the RFR email I have been thinking that we might want to
>> have some check there either way to see if the type being allocated is
>> actually complete. e.g.:
>>
>>     if(type.isPartial()) {
>>         throw new IllegalArgumentException("Can not allocate
>> incomplete type: " + type);
>>     }
>>
>> However, the `isPartial()` check does not only check if a type is
>> incomplete. For instance for a pointer; the Address layout isPartial()
>> method is inherited from Value which delegates to it's `content`, if
>> it has one, to do the `isPartial()` check [1]. This makes sense if we
>> have just a Value, but imho Address should override isPartial() to
>> always return `true`, since we can always allocate a pointer (just
>> maybe not what it points to).
>>
>> What do you think?
>>
>> Jorn
>>
>> [1] :
>> http://hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/java/foreign/layout/Value.java#l100 
>>
>>
>> Maurizio Cimadamore schreef op 2019-01-21 11:52:
>>> Hi,
>>> I've been wanting to solve this in the past, but I stumbled upon the
>>> fact that we could not always ensure 'eager' resolution when calling
>>> LayoutType.ofStruct. This is something that eventually should be fixed
>>> - the situations where such resolution cannot happen are always caused
>>> by the fact that jextract is generating cross-header symbolic
>>> references - this is another problem that could be addressed by
>>> switching to a library-per-extraction approach, where then all
>>> symbolic references will become self-contained.
>>>
>>> In the meantime, your idea of adding a 'tryResolve' is a good one;
>>> I'll do some more tests on my side, to make sure that everything is
>>> ok, and then I'll approve
>>>
>>> Cheers
>>> Maurizio
>>>
>>> On 19/01/2019 15:16, Jorn Vernee wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217380
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217380/webrev.00/
>>>>
>>>> Thanks,
>>>> Jorn