[foreign] RFR 8218742: Refine Scope API

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Feb 14 15:21:13 UTC 2019 

Tests passed on all platforms - pushed

thanks!
Maurizio

On 14/02/2019 09:25, Sundararajan Athijegannathan wrote:
> Looks good.
>
> -Sundar
>
> On 13/02/19, 7:39 PM, Maurizio Cimadamore wrote:
>> Here's another iteration
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8218742_v4/
>>
>> Changes:
>>
>> * Added security checks to Scope.globalScope, Libraries.libraryScope
>>
>> * Removed security check from Scope.fork (since you had to be able to 
>> grab a scope somewhere to get there in the first place)
>>
>> * Changes back semantics of library scope - after a discussion with 
>> Sundar, it turns out that a class-based semantics is much closer to 
>> what's actually happening, since the library will be loaded in the 
>> process only once, so it's not like every new library instance will 
>> get e.g. globals in different regions of memory
>>
>> * I would have liked to tweak back the signature of 
>> Libraries.libraryScope as this:
>>
>> Scope libraryScope(Class<?> libraryClass)
>>
>> But I'm afraid that's not possible with current impl - we store all 
>> bound implementation in a ClassValue, and there's no way to 'peek' 
>> into a ClassValue to see if there's a mapping for a given class - 
>> calling ClassValue::get will actually end up binding the library - 
>> that means that in principle a client might do:
>>
>> Scope s = Libraries.libraryScope(FooLib.class);
>>
>> And this will create a new binding implementation, return a fresh 
>> library scope - but then, since the library is not referenced, the GC 
>> will kick in and the Cleaner will close the scope immediately. That's 
>> too weird of a side-effect for a method that is supposed to provide 
>> access to an _already bound_ library.
>>
>> For this reason, I left the signature as it was. If you want a 
>> library scope, you need to pass an object - which is a proof that the 
>> library has been bound.
>>
>> Maurizio
>>
>> On 12/02/2019 12:29, Maurizio Cimadamore wrote:
>>>
>>> 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 
>>>>>>>>>>>

More information about the panama-dev mailing list