[foreign] RFR 8218742: Refine Scope API

Sundararajan Athijegannathan sundararajan.athijegannathan at oracle.com
Thu Feb 14 09:25:39 UTC 2019 

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