[foreign] RFR 8218742: Refine Scope API

Tue Feb 12 12:20:26 UTC 2019

Scope.globalScope() and Libraries.libraryScope() methods should do 
security check.

ScopeTest.java test could be expanded to add the relevant security 
access check testing.

Other than that, looks good.

-Sundar

On 12/02/19, 1:31 AM, Jorn Vernee wrote:
> 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 
>>>>>>>