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

[Jorn Vernee]

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