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

[Jorn Vernee]

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