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

[Maurizio Cimadamore]

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.

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.

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