[foreign] RFR 8218742: Refine Scope API

[Jorn Vernee]

Cool! Looks good.

Cheers,
Jorn

Maurizio Cimadamore schreef op 2019-02-11 20:12:
> Here's the latest iteration:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8218742_v3/
> 
> I added the test, which uncovered some issues in two tests, namely
> FunctionAccessTest and PointerScopeTest; the first case was a real
> issue, as the code was trying to set a global callback with a callback
> whose scope was more narrow - hence the issue. The other test was
> attempting something similar and, in doing so, it revealed a
> difference in the exception being generated. Both tests have been
> fixed to use the 'right' scope to create the callback to be set.
> 
> Maurizio
> 
> On 11/02/2019 18:26, Maurizio Cimadamore wrote:
>> 
>>> 
>>> Should there be a test added for the Scope inference? Specifically it 
>>> seems useful to test that scopes are inferred correctly when 
>>> extracting a Resource from a Struct. As well as a test to check that 
>>> an exception occurs when trying to put a resource into a struct while 
>>> it doesn't have the correct Scope. The tricky part for the latter is 
>>> that this works differently for Struct/Array and Pointer/Callback. 
>>> For Struct & Array, the resource part, i.e. the block of memory they 
>>> manage, is copied into the Struct's memory (see References [2, 3]), 
>>> so there should be no Scope requirements. But for Pointer & Callback, 
>>> the resource part is not copied, so there should be Scope 
>>> requirements for those 2. But all 4 Resource types should get the 
>>> Struct's scope when extracting the resource. I don't think all the 
>>> scenarios are covered by the current tests.
>> 
>> Yes a test would be great - and actually the one you have in [4] looks 
>> awesome. Thanks!
>> 
>> I'll add it and fix whatever issue I find as part of next iteration
>> 
>> Maurizio
>> 
>>> 
>>> FWIW, I have made a test you could use [4]. One of the test cases is 
>>> currently failing because the Scopes of Callbacks are not being 
>>> checked in References.OfFunction::set. Adding a check similar to the 
>>> one in Reference.OfPointer::set should fix the failure.
>>> 
>>> Cheers,
>>> Jorn
>>> 
>>> [1] : 
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8218742_v2/test/jdk/com/sun/tools/jextract/staticForwarder/StaticForwarderTest.java.sdiff.html
>>> [2] : 
>>> http://hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/jdk/internal/foreign/memory/References.java#l487
>>> [3] : 
>>> http://hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/jdk/internal/foreign/memory/References.java#l537
>>> [4] : 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/structscopetest/ResourceStructTest.java
>>> 
>>> Maurizio Cimadamore schreef op 2019-02-11 16:18:
>>>> Hi, here's v2:
>>>> 
>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8218742_v2/
>>>> 
>>>> I addressed most of the comments, see below
>>>> 
>>>> On 11/02/2019 13:46, Jorn Vernee wrote:
>>>>> Hi Maurizio,
>>>>> 
>>>>> The patch does not apply cleanly after JDK-8218669 [1] was merged. 
>>>>> I'm getting some merge failures in References.java
>>>>> 
>>>>> About ScopeImpl;
>>>>> 
>>>>>   - The parent scope field is called `owner`, but the accessor is 
>>>>> called `parent()`, I'd say go with either "parent" or "owner" as a 
>>>>> name for both.
>>>> fixed
>>>>> 
>>>>>   - Also, There are a bunch of casts to ScopeImpl, mostly in the 
>>>>> implementation of ScopeImpl::merge. Should the field's type be 
>>>>> changed to ScopeImpl instead, and have 1 cast in the constructor?
>>>> fixed
>>>>> 
>>>>> - Changes in jextract/staticForwarder/StaticForwarderTest.java 
>>>>> don't seem to be needed?
>>>> Not sure what you mean - the static forwarded class is a place where
>>>> we can have an accessor for the library scope - that's what the
>>>> changes do (and the test checks that such accessor works).
>>>>> 
>>>>> - Also, Scope inference is in the proposal, but it doesn't seem to 
>>>>> be implemented with this patch. I guess that will come later?
>>>> 
>>>> In this revision I've added inference - in the sense that when 
>>>> calling
>>>> Reference.OfPointer::get we know infer the scope of the extracted
>>>> pointer to be the same as that of the pointer we're using for
>>>> dereference.
>>>> 
>>>> I believe there's some more work to do, especially for native
>>>> functions - ideally you'd like things coming out of a native 
>>>> function
>>>> to use the library scope - but this is problematic because (1) when 
>>>> we
>>>> create the invokers MH we do not necessarily have a library scope
>>>> available (this is the same issue as with globals, which is made 
>>>> more
>>>> complex by the fact that method handles are created by SystemABI and
>>>> that API does not depend on scopes - nor we want it to, I believe).
>>>> Another issue is that for (2) callback arguments (e.g. think of a
>>>> callback accepting a pointer) we'd like again for the scope to be
>>>> inferred as the library scope that's making the callback call - but
>>>> I'm not sure that's even possible, since we jump into the callback
>>>> from C code, which knows very little about scopes.
>>>> 
>>>> The current patch uses an unchecked scope in both cases.
>>>> 
>>>> P.S.
>>>> 
>>>> I'm not 100% sure that the new meaning of library scope - as a scope
>>>> associated with a specific _instance_ of a library (as opposed to
>>>> class) is more useful or harmful - it certainly makes a lot of the
>>>> code generation harder because now all handles we use might, in
>>>> principle, depend on scope which is stored in an instance field of 
>>>> the
>>>> generated class.
>>>> 
>>>> Maurizio
>>>> 
>>>> 
>>>>> 
>>>>> Rest looks good,
>>>>> Jorn
>>>>> 
>>>>> [1] : https://bugs.openjdk.java.net/projects/JDK/issues/JDK-8218669
>>>>> 
>>>>> Maurizio Cimadamore schreef op 2019-02-11 14:03:
>>>>>> Hi,
>>>>>> this is an official RFR for the changes discussed in the writeup 
>>>>>> in
>>>>>> [1] and in the subsequent thread [2].
>>>>>> 
>>>>>> Webrev:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8218742/
>>>>>> 
>>>>>> I've updated the examples document to reflect the API changes, as 
>>>>>> well
>>>>>> as polished the code, fixup javadocs etc.
>>>>>> 
>>>>>> The tests pass on all platforms.
>>>>>> 
>>>>>> Cheers
>>>>>> Maurizio
>>>>>> 
>>>>>> [1] - http://cr.openjdk.java.net/~mcimadamore/panama/scopes.html
>>>>>> [2] -
>>>>>> https://mail.openjdk.java.net/pipermail/panama-dev/2019-January/003926.html