[foreign] RFR: 8229857: scope inference on pointer to pointer is not friendly

[Henry Jen]

Updated webrev[1]

> On Sep 25, 2019, at 3:03 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Hi Henry - some comments:
> 
> * I think I'd still prefer, in terms of code hygiene, to have a static, well-known UNCHECKED scope, rather than just null. Doesn't cost anything in terms of memory, and you still have a quick way to check (but then we have to protect clients not to use the unchecked scope to allocate themselves)
> 

Added an anonymous class for UNCHECKED scope won’t be able to call allocate methods.

> * I'm on the fence with function pointers and with their relationship with assertScope. The idea behind management of VM stubs is that the stub keeps a weak reference to the upcall handler that should be used to make the Java upcall; the upcall handler is kept there by the scope (a scope keeps a list of all the stubs allocated within it with Scope::allocateCallback). When the scope is closed, the list is cleared, the upcall handler becomes unreachable, and this triggers a cleaner which will get rid of the sub (this cleaner is registered when clients call SystemABI::upcallStub - which is in turn called by Scope::allocateCallback).
> 

We had something similar(JavaSymbol) when we did ABI implementation, so that we can get back the Java side object with the long address when it does a round-trip. For the purpose to get the correct Scope, we would need something like that.

Since we agreed that to Pointer from native side won’t have a scope, and developer need to make sure such address from native side is valid, so in the case of passing callbacks down, developer should make sure the scope is not closed prematurely.

So I removed assertScope() as suggested, as that’s really not adding any value.

[1] http://cr.openjdk.java.net/~henryjen/panama/8229857/2/webrev/

Cheers,
Henry

> In other words, if you allocate a callback in a scope, then save the callback onto some native struct, then close the scope, there's no guarantee that the callback will still be there to call (depending on how soon, or late the GC calls the cleaner).
> 
> So, in a way, if you read a function pointer from native that originally comes from Java (e.g. a callback) and you give it the 'unchecked' scope - I think one might have an expectation that the stub will be there forever, which is fundamentally not the case. The stub is still logically associated with its originating scope, and will cease to exist when the original scope has gone away.
> 
> To me, this seems to suggest that for callbacks our strategy has to be more nuanced:
> 
> - if you read a function pointer that doesn't come from Java (e.g. is not a lambda passed to some native function) - then returning an unchecked function pointer is fine
> 
> - if you read a function pointer that comes from Java, its original scope should be restored (and probably checked again) - after all the stub is saved _inside_ the stub; I believe we could similarly save a weak reference to the scope in there w/o wasting too much space. With pointers, of course, we don't have this luxury (*).
> 
> In other words, I think unmanaged should be == native.
> 
> (*) I half-lied when I said that with pointer we don't have luxury to do something similar - in fact we do, but we don't currently do much in that direction; that is, a native scope is, essentially, also an allocator. Since this allocator allocates big chunks of memory and then allocates from there, there is in principle nothing stopping us from saving a scope weak reference into a reserved portion of the allocated memory - and then have some scheme to get back the scope refernce given an address that falls within the allocator range. This is hard to do now, because all the allocations we do are independent mallocs, but might not be so hard to do if we allocated memory in a different way; in any case this is worth keeping in the back of our minds for later.
> 
> Maurizio
> 
> 
> On 25/09/2019 03:52, Henry Jen wrote:
>> I revised the webrev[1] based on feedbacks, here is a quick summary:
>> 
>> - Stick with a single Pointer interface, with a isUnmanaged() predicate to allow check if the pointer is not associated with a Scope
>> - The method assertScope() still with same limitation that only allow an unmanaged Pointer to be associate with a Scope. It’s developer’s responsibility to make sure it’s proper. However, use of this method is mostly unnecessary.
>> - Relax the pointer assignment requirement derived based on scope inference. This is somewhat significant, but as we seems to agree that the scope inference is not working well, we take this out from Pointer API and expect usage not involving native code but only native memory be using MemAccess API instead. Case in point, when a native function returns a struct by value, the struct has it’s own scope, while we still need to work out a better approach for that, the limitation will create a subtle difference that’s not obvious for developer, see ScopeTest.java:214-215.
>> - Same scope inference change apply to Callback as well
>> 
>> Also I include another small change worth mentioning,
>> - Scope.allocateCString will allow null and return a Pointer.ofNull() in that case instead of cause NPE. This make it easier to make call to native functions more transparent where null pointer is accepted. We can keep it strict, but that would make more verbose code with null check when calling native functions take a null.
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/panama/8229857/1/webrev/
>> 
>> Cheers,
>> Henry
>> 
>> 
>>> On Aug 28, 2019, at 12:56 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>> 
>>> 
>>> On 28/08/2019 18:23, Maurizio Cimadamore wrote:
>>>> Do we agree on the description of the issue here? Then we can talk on how to fix it.
>>> Since we agree on the problem statement, let's dissect the problem a little more.
>>> 
>>> In my view this discussion raises the issue that there are two (very different) kind of pointer - I'll be using the terminology managed/unmanaged.
>>> 
>>> * managed pointers are created in Java code, typically via a Scope::allocate operation. They always have a finite region size, and a finite temporal range.
>>> 
>>> * unmanaged pointers are created by native code (or synthesized by the Panama runtime e.g. during struct access). They have an unknown region size and unknown range.
>>> 
>>> To me, this suggests (at least a design level) that this distinction between managed and unmanaged should perhaps be surfaced in the API. That is, there should be a top type Pointer<X> which is the common root for all pointers, both managed and unmanaged. As such it will NOT feature a scope() method.
>>> 
>>> Unmanaged or UncheckedPointer will only be available in the binder internals, but not available outside. They are not attached to any scope.
>>> 
>>> Then you have a ManagedPointer underneath, which does have a scope() method. These are the pointers returned by the Scope allocation methods.
>>> 
>>> As long as you remain in the ManagedPointer realm, you are safe - ownership keeps your pointer in check and so forth. We could even apply scope inference, if a struct's return type is ManagedPointer<X>, and get same strict semantics we have now. But, in the general case, jextracted types will be unmanaged (e.g. just Pointer<X>), meaning that to go from an unmanaged pointer to a managed one some magic trick is needed (there could be an API point for this... or not), in which case all safety guarantees are abandoned (e.g. it will be possible to crash the VM by assigning the wrong scope to an unmanaged pointer and then dereferencing it at the wrong time).
>>> 
>>> Now, in terms of implementation - we could do exactly as I describe, and add a 'managed pointer' interface that extends from pointer (and, I guess, same for arrays?).
>>> 
>>> Or, we could do a lump move, and use same interface for both managed and unmanaged case, plus a predicate which tells us if this is a managed pointer/array (or infer the managed-ness from the fact that there's no owning scope). And then add a projection which attaches some scope/size to an unmanaged pointer, therefore making it managed (but this would be an unsafe operation in general).
>>> 
>>> Maurizio
>>> 
>>>