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

[Jorn Vernee]

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

Actually, now that you mention it, this is only testing for a 
fits-in-register struct, not for one being passed by pointer, which is 
the case I was trying to solve. I'll take a look at adding more tests 
tomorrow (getting tired).

Jorn

Jorn Vernee schreef op 2019-01-21 22:31:
> 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.
> 
> 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