[foreign] RFR 8218742: Refine Scope API

Wed Feb 13 14:09:18 UTC 2019

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