[foreign] RFR 8218742: Refine Scope API
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Feb 11 17:47:09 UTC 2019
On 11/02/2019 17:41, Jorn Vernee wrote:
> Jorn Vernee schreef op 2019-02-11 18:32:
>>>> - 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).
>>
>> Sorry, I'm just talking about the test here. It's creating a new Scope
>> that doesn't seem to be used [1]
>
> Aha, I got you. The creation of the scope _is_ the test.
:-)
Thought about adding a comment - will do so
Maurizio
>
> Jorn
>
> Jorn Vernee schreef op 2019-02-11 18:32:
>>>> - 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).
>>
>> Sorry, I'm just talking about the test here. It's creating a new Scope
>> that doesn't seem to be used [1]
>>
>>>> - 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.
>>
>> Ok, thanks, something like that is what I was expecting to see for
>> Scope inference. I think most of the relevant code ends up delegating
>> to Reference.OfPointer. The scope inference for native calls is
>> tricky, like you say, so it seems good to leave it as-is for now, and
>> make progress in other areas first.
>>
>> 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.
>>
>> 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
>>>>>
More information about the panama-dev
mailing list