[foreign] RFR 8218742: Refine Scope API

[Maurizio Cimadamore]

>
> 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 
>>>>