[foreign] RFR 8218742: Refine Scope API

Tue Feb 12 12:29:57 UTC 2019

On 12/02/2019 12:20, Sundararajan Athijegannathan wrote:
> Scope.globalScope() and Libraries.libraryScope() methods should do 
> security check.

Whoops - these got wiped out accidentally

I agree we should bring them back

Thanks
Maurizio

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