[foreign] RFR 8217420: Have UniversalNativeInvoker and UniversalUpcallHandler create (and close) the scopes used by boxing code

[Maurizio Cimadamore]

On 21/01/2019 21:31, Jorn Vernee wrote:
> Maurizio Cimadamore schreef op 2019-01-21 22:16:
>> Hi Jorn,
>> I think I agree with the spirit of the fix. I like that this makes the
>> scopes stricter - which ended up uncovering some issues.
>>
>> Again, I have to point out that this fix only restrict things for
>> universal invoker mode - I believe that when using the direct
>> invocation scheme, parameters to upcalls will be passed in leaky
>> scopes, which will not reveal the clang-related failure.
>
> Sorry about that, I wasn't sure before if I was supposed to make 
> changes to the direct invoker as well, since I would have no way of 
> testing those changes locally. (I've put it on my checklist for the 
> future).
>
>> I think I'd like to see a specific test for lifecycle of callback
>> args, so that we can easily run it with all possible invoker modes and
>> make sure it passes.
>
> I added a test for that (CallScopeTest), is there something else you'd 
> want to have tested? AFAICT this lifecycle problem currently only 
> exists with structs.

That's good - I missed that earlier. I guess we need to make sure that 
the same semantics is applied on all platforms; I'm a bit uncertain on 
how that's gonna work for the direct invoker (or the linkToNative 
invoker, for what it's worth) - as these invokers are based on the 
assumption that arguments can be converted into their native form on the 
fly - I'm not sure in those invoker it would be easy to do something 
such as creating a call-wide scope (in either direction).

I can try and take a look. I guess I could use the 
MethodHandles.tryFinally combinator to make sure the scope is closed 
after the call (and also inject a Scope prefix parameter on all adapter 
handles), but I'd also have to check what happens to performances.

Maurizio

>
> Jorn
>
>> Maurizio
>>
>> On 21/01/2019 18:13, Jorn Vernee wrote:
>>> Hi, another patch :)
>>>
>>> From the bug description:
>>>
>>> Currently, when (un-)boxing code needs to do an allocation a Scope 
>>> is created for that, and this Scope is then leaked.
>>>
>>> For an up-/down-call there are 2 kinds of scopes; 1) for values 
>>> being passed as arguments by value, which are valid for the duration 
>>> of the call. 2) for values being returned, which are up to the 
>>> caller to manage.
>>>
>>> We can do better than the current scheme of always leaking, by 
>>> having UniversalUpcallHandler and UniversalNativeInvoker create both 
>>> of these types of scopes, pass them to boxing code, and then close 
>>> scopes of type 1. when the call is finished.
>>>
>>> This will reduce the amount of memory being leaked, and could also 
>>> help identify bugs where references to temporary objects are leaked.
>>>
>>> ---
>>>
>>> After implementing this, it turns out that UniversalUpcallHandler 
>>> only needs a Scope for the arguments being passed to the java method 
>>> being called. For returns there should be no allocations; either the 
>>> value is being passed in registers, or written to a pointer passed 
>>> in by the caller.
>>>
>>> I did discover a problem with this patch in the TestJextractFFI 
>>> patch code:
>>>
>>>     public Stream<Cursor> children() {
>>>         final ArrayList<Cursor> ar = new ArrayList<>();
>>>         // FIXME: need a way to pass ar down as user data d
>>>         try (Scope sc = Scope.newNativeScope()) {
>>>             LibClang.lib.clang_visitChildren(cursor, 
>>> sc.allocateCallback((c, p, d) -> {
>>>                 ar.add(new Cursor(c));
>>>                 return LibClang.lib.CXChildVisit_Continue();
>>>             }), Pointer.nullPointer());
>>>             return ar.stream();
>>>         }
>>>     }
>>>
>>> According to C semantics, the argument to the lambda `c`, which is a 
>>> CXCursor struct being passed in by-value, is only valid for the 
>>> duration of the call. The Windows ABI depends on this fact, because 
>>> it allocates the struct on the stack of the caller, and then passes 
>>> a pointer to the callee (the lambda in this case). I think on SysV a 
>>> by-value struct is always passed in register + stack?
>>>
>>> But, the `c` argument is being leaked from the lambda, by wrapping 
>>> it in a Cursor and storing it in a list in the enclosing scope. When 
>>> the call to the lambda ends, the struct is deallocated and the 
>>> pointer that is stored on the Java side is now dangling. I've 
>>> initially solved the crash this was causing by defensively copying 
>>> the struct in the Windows boxing code (instead of just wrapping the 
>>> passed pointer), but I realized this also means that a user would 
>>> have to manually release a struct passed into an upcall to avoid a 
>>> memory leak.
>>>
>>> We have 2 options here:
>>>
>>> 1.) Go with C style. Struct arguments are valid only during an 
>>> upcall, and we have to manually copy them if we want to 'leak' them 
>>> from the scope of the upcall.
>>> 2.) Go with status quo. Struct doesn't need to be copied manually 
>>> when saving it outside the scope of the upcall. The copy is 
>>> defensively done in Windows boxing code instead, and sometimes 
>>> redundant. Only on Windows (for now), struct arguments to an upcall 
>>> have to be manually released to avoid a memory leak.
>>>
>>> I think 1 is the better option here since this adheres to the 
>>> expectations native code has when calling into Java. I've reflected 
>>> that in the patch as well, but of course I'm interested in hearing 
>>> other opinions as well :) Note that the crash I was seeing before no 
>>> longer happens since the Scope used in the boxing code is now closed 
>>> after the call, and we get an exception instead.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217420
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8217420/webrev.00/
>>>
>>> Cheers,
>>> Jorn