[foreign] RFR: Fix UndefinedLayoutException exception message

[Jorn Vernee]

Maurizio Cimadamore schreef op 2019-01-29 17:09:
> On 29/01/2019 15:57, Jorn Vernee wrote:
>> Maurizio Cimadamore schreef op 2019-01-29 16:46:
>>> On 29/01/2019 15:19, Jorn Vernee wrote:
>>>> Maurizio Cimadamore schreef op 2019-01-29 15:59:
>>>>> Resolving references across interfaces is not expected to work
>>>>> reliably. This is the primary reason which sends us towards an
>>>>> extraction model that is library-centric, rather than 
>>>>> header-centric,
>>>>> so that all references in a library should be self-contained.
>>>> 
>>>> I've been using a mitigating patch that also scans methods of a 
>>>> Struct implementation in LayoutResolver when scanning a struct. 
>>>> Under the assumption that a type's layout is defined on that type 
>>>> itself.
>>>> 
>>>>> As for your test, it seems like you can get the same exception 
>>>>> simply
>>>>> by having a single struct interface whose layout has some dandling
>>>>> unresolved layout (e.g. an unresolved layout whose layout 
>>>>> expression
>>>>> is not defined anywhere)?
>>>> 
>>>> The tricky part is triggering the resolution of the layout through 
>>>> the public API. Trying to allocate a struct will not resolve the 
>>>> struct's layout. The only place I found that does this is either the 
>>>> test case I've used, where the dangling layout is in a 
>>>> @NativeFunction descriptor, or in StructImplGenerator's constructor, 
>>>> which is called when de referencing a pointer to a struct.
>>> 
>>> So, I guess what I'm saying is:
>>> 
>>> @NativeStruct("[ ${foo}(foo) ]")
>>> interface BadStruct extends Struct<BadStruct> {
>>>     @NativeGetter("foo")
>>>     Foo foo();
>>> 
>>>     interface Foo { }
>>> }
>>> 
>>> Shouldn't this fail when trying to generate the struct 
>>> implementation?
>>> (e.g. Scope.allocateStruct(BadStruct.class))
>> 
>> No, that will fail with "UnsupportedOperationException: bitsSize on 
>> Unresolved"
> 
> Then try with
> 
> scope.allocate(LayoutType.ofStruct(BadStruct.class)).get().foo()

Same there, that also calls allocateInternal.

Another way to trigger the exception could be something like;

```
try (var scope = Scope.newNativeScope()) {
     Pointer<?> ptr = scope.allocate(NativeTypes.VOID);
     ptr.cast(LayoutType.ofStruct(BasStruct.class)).get(); // trigger 
resolution
}
```

But that didn't seem like a realistic enough test. So, I went with the 
case in which I was seeing the exception; which is a cross-header layout 
reference.

Jorn

> Maurizio
> 
>> 
>> Scope.allocateStruct(BadStruct.class) will call allocateInternal first 
>> which throws that UOE, only after that would the implementation be 
>> generated when calling get() on the returned pointer;
>> 
>> ```
>> private <T> BoundedPointer<T> allocateInternal(LayoutType<T> type, 
>> long count, int align) {
>>     ...
>>     long size = Util.alignUp(type.bytesSize(), align); // !!! calling 
>> bitSize() on Unresolved throws UOE
>>     ...
>> }
>> 
>> @Override
>> public <T extends Struct<T>> T allocateStruct(Class<T> clazz) {
>>     // FIXME: is this alignment needed?
>>     return allocate(LayoutType.ofStruct(clazz), 8).get(); // call to 
>> 'get()' triggers struct generation, and resolution of struct's layout.
>> }
>> ```
>> 
>> Jorn
>> 
>>> Maurizio
>>> 
>>>> 
>>>> Of course, I could also just use 
>>>> `LayoutResolver.get(Object.class).resolve(Layout.of("${Undefined}"));`. 
>>>> Would that be good enough?
>>>> 
>>>> Jorn
>>>> 
>>>>> Maurizio
>>>>> 
>>>>> On 28/01/2019 14:52, Jorn Vernee wrote:
>>>>>> Ok, it took a while to find a good test case. I was seeing the 
>>>>>> exception due to a cross-header layout reference, so that's also 
>>>>>> the test case I added. It seems that that is currently the only 
>>>>>> realistic use-case where this exception is being thrown. I believe 
>>>>>> not being able to resolve cross-header layout references is a 
>>>>>> known issue as well?
>>>>>> 
>>>>>> Updated webrev: 
>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/fixmessage/webrev.01/ 
>>>>>> Jorn
>>>>>> 
>>>>>> Jorn Vernee schreef op 2019-01-28 14:16:
>>>>>>> Yes, I will do that. (wasn't sure if it was useful enough).
>>>>>>> 
>>>>>>> Jorn
>>>>>>> 
>>>>>>> Sundararajan Athijegannathan schreef op 2019-01-28 14:12:
>>>>>>>> Is it possible to add a test with invalid (unresolvable) name
>>>>>>>> reference in a layout descriptor and check the exception 
>>>>>>>> message?
>>>>>>>> 
>>>>>>>> -Sundar
>>>>>>>> 
>>>>>>>> On 28/01/19, 6:36 PM, Jorn Vernee wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> JDK-8217245 changed the syntax for Unresolved layouts from 
>>>>>>>>> `$(Name)` to `${Name}`. This also means Unresolved now has a 
>>>>>>>>> dedicated `layoutExpression` field instead of relying on the 
>>>>>>>>> name annotation.
>>>>>>>>> 
>>>>>>>>> LayoutResolver.UndefinedLayoutException is still using the name 
>>>>>>>>> annotation in the exception message, which is now incorrect. 
>>>>>>>>> This small patch fixes that, and changes it to use 
>>>>>>>>> Unresolved::layoutExpression().
>>>>>>>>> 
>>>>>>>>> Please review.
>>>>>>>>> 
>>>>>>>>> Webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/fixmessage/webrev.00/ 
>>>>>>>>> Cheers,
>>>>>>>>> Jorn