[foreign] RFR 8218742: Refine Scope API

Mon Feb 11 17:32:00 UTC 2019

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