RFC: Refactoring SystemABI

[Henry Jen]

Please review webrev.03[1].

Revert changes in NMT and Symbol, SystemABI upcall/downcall use Symbol.

JavaSymbol remains as a construct that extract  MH from various forms, call SystemABI.upcallStub to obtain a symbol, wrap it up with better name if available, and publish itself in the Pointer->Symbol registry so Callback can recover the stub from a pointer.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.03/webrev/



> On Dec 7, 2018, at 11:34 AM, Henry Jen <henry.jen at oracle.com> wrote:
> 
> On Dec 7, 2018, at 9:34 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>> 
>> Summarizing from the offline discussion:
>> 
>> 1) Regarding safety, you raise a broad (and legitimate) point which seems to be: SystemABI is too unsafe, we need some intermediate API in the middle (such as Symbol). But once you can turn a pointer into executable code (which you can do with both approaches), safety is already down on the drain. Example: what if I pass a 'getter' NativeMethodType to Symbol::methodType when in reality the symbol describes a function? Whoops.
>> 
> 
> True, my other concern is that SystemABI is more abroad if we make that public, thing like Layout and CallingConvention. Anyway, guess that’s no harm anyway.
> 
>> 2) Symbol::methodHandle seems to distort the NativeMethodType API, which should be a mere collection of layout types and now has become something more. Vararg-ness is part of the signature; being a getter/setter is not - it's a semantic property of what the function does. So by doing that move we are conflating the static description with the dynamic one. Which is why I believe the simpler approach described with SystemABI::upcall/Downcall accepting/returning Symbol felt a good compromise.
>> 
>> 3) The binder, that is, high level classes such as CallbackImpl should *not* rely on SystemABI to output things like JavaSymbols. A symbol is an opaque representation of some native entry point - and it's up to the ABI to cough up one; from the binder perspective, a symbol is just a symbol - if the binder wants to call it, only option is via SystemABI::downcallHandle.
>> 
>> Considering this, I suggest we go back to what has been discussed few days ago:
>> 
>> - use Symbol in the signatures of SystemABI::upcall/downcallHandle
>> - remove Kind from NMT
>> - remove Symbol::methodHandle
>> - tweak the implementation so that the binder doesn't depend on the particular shape of Symbol produced by the ABI
>> - also tweak freeUpcallStub to take a Symbol
>> 
> 
> Agreed.
> 
> Cheers,
> Henry
> 
> 
>> Thanks
>> Maurizio
>> 
>> 
>> On 07/12/2018 16:24, Henry Jen wrote:
>>> On Dec 7, 2018, at 3:45 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>> I'm lost :-(
>>>> 
>>>> I think we agreed to use Symbol in both upcall/downcall methods...
>>>> 
>>>> Having downcall handle accept just any pointer is IMHO a step back. It's much easier to shoot yourself in the foot. Symbol was fine here.
>>>> 
>>>> We also discussed how, for consistency, upcallStub needed to return a symbol too - if internally you want to call it JavaSymbol, fine - but I think from a public API perspective the two methods should be:
>>>> 
>>>> public MethodHandle downcallHandle(CallingConvention cc, Symbol s, NativeMethodType nmt);
>>>> public Symbol downcallHandle(CallingConvention cc, MethodHandle target, NativeMethodType nmt);
>>>> 
>>>> 
>>> Pointer is the lowest level and proper for SystemABI as where it sit.
>>> Symbol is a level higher. It’s a matter of whether we want to separate those two layers or not.
>>> 
>>> 
>>>> 
>>>> 
>>>> Let's also try to keep this simple - putting the MethodHandle logic in Symbol is something that might have some value, but (1) is non essential (given the above API you can always get a MethodHandle for a Symbol using SystemABI::downcallHandle) and (2) has not been discussed in this thread.
>>>> 
>>> Public API is only JavaExports and Symbol, everything else is internal as SystemABI, JavaSymbol.
>>> As I said before, I don’t see why SystemABI should be external, as that’s a very important piece and likely only builder of JDK will use. Unless it is as an service API, which I don’t think is needed.
>>> 
>>> Instead, Symbol is exposed as public API to dealing with calls, either it’s upcall or downcall.
>>> 
>>>> Also, we should try to reduce the surface area in terms of public API and this patch seems to go in the direction of adding two new types (not yet public but...): JavaSymbol and JavaExports, both of which I don't think pass the test for 'need to be in public API'. I think a lot of the rationale behind these classes is to avoid creating a Callback wrapper in cases where the underlying symbol is coming from java - and I think it's not worth making the API more complex to solve a problem that never comes up in practice (yes, you can call the callback stub you have created from Java, but in reality, how frequent that would be?).
>>>> 
>>>> 
>>>> In other words:
>>>> 
>>>> 1) SystemABI::upcallStub returns a Symbol
>>>> 2) CallbackImpl saves that symbol somewhere
>>>> 3) CallbackImpl::asFunction creates a callback wrapper (using CallbackImplGenerator) and pass it the symbol it has
>>>> 4) The implementation generated by CallbackImplGenerator will take care to do the downcall using SystemABI (passing the symbol). There will be two cases here:
>>>> 4a) the Symbol is the one obtained from SystemABI::upcallHandle - in that case the downcall logic will do something special and bypass the native call
>>>> 4b) the Symbol is some opaque entry point created by the binder (e.g. when reading a function pointer field in a struct); in that case a full downcall happens
>>>> 
>>>> I think this is self-consistent, and does not require any extra API. What's better, we completely delegate to the ABI the task of recognizing 'its own' upcall symbols, which means that each ABI implementation could have a different notion of upcall symbols - and some ABIs might even decide not to optimize (4a) and always go native.
>>>> 
>>>> 
>>> Sure we can combine those two layers and have SystemABI instead. With Symbol as the public API make it’s more friendly to add constructs for different sources. While SystemABI can only take MH and will need other helpers. Also the main benefit to have meaningful names, rather than make up in upcallHandle.
>>> 
>>>> Also, please no Kind on NativeMethodType - this should be about types - and the new kind turns it more into a kind of 'method handle'.
>>>> 
>>>> 
>>> Sort of, but I don’t feel it’s too bad. It’s describing a native method, Kind is a metadata just as varargs. We simply expand it to cover variable.
>>> 
>>>> To summarize, it feels (probably because of the back and forth made during the discussion last week, and that's my fault too!) that a bad turn has been made somewhere. I suggest we go back to a simpler state and simply define the ABI upcall/downcall methods with the signatures shown above.
>>>> 
>>>> 
>>>> 
>>>> Thanks - I know this feedback will probably sound harsh, and I also know you put a lot of effort into this; but we need this API to settle into a low energy state where SystemABI only depends on a bunch of public types (Pointer, Symbol, NativeMethodType, LayoutType). I think this is crucial to get this API right - the more API points we introduce, the more dependencies between the ABI and the binder, and we want to keep those to a minimum!
>>>> 
>>> Are you talking about internal implementation or public API? I don’t think we have more public API than choose to expose SystemABI, it actually is a subset. Again, I don’t think SystemABI should be exposed as public API.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>>> Maurizio
>>>> 
>>>> 
>>>> 
>>>> On 07/12/2018 08:03, Henry Jen wrote:
>>>>> Please have a look at updated webrev.02[1]
>>>>> 
>>>>> Quick summary:
>>>>> 
>>>>> 1. SystemABI upcall/downcall are dealing with Pointer, not Symbol
>>>>> 2. abi.sysv.x64 package is abi.x64.sysv as suggested by Jorn
>>>>> 3. Library.Symbol becomes the public API presenting a consistent view for native and java symbol. From which, one can get  MH for java call and Pointer for native function pointer.
>>>>> 4. I haven’t move invokers into abi specific folder as I think most of them would be share code, let me know particular file you think should be moving, we can always do that later though.
>>>>> 
>>>>> There are still thing to improve, but the Symbol approach come together nicely. I haven’t expose JavaSymbol just yet, it is now to be managed via Scope/Callback which manage the life-cycle.
>>>>> 
>>>>> In a way, JavaSymbol is the UpcallHandle we had earlier.
>>>>> 
>>>>> JavaExports was created to be the public API to obtain JavaSymbol, with API for each form of JavaSymbol construction with unbounded pointer and user will need to manage the lifecycle of the symbol. I moved them back into as JavaSymbol.of(..). If we like this, then we can open it up.
>>>>> 
>>>>> It currently serve the global registry of native address to JavaSymbol lookup, which I am debating whether should be move into CallbackImpl.
>>>>> 
>>>>> Let me know what you think. There are still some cleanup can be done, also if we like the Symbol approach, we should add some test cases.
>>>>> 
>>>>> Cheers,
>>>>> Henry
>>>>> 
>>>>> [1]
>>>>> https://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.02/webrev/
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Dec 4, 2018, at 10:43 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com>
>>>>>> wrote:
>>>>>> 
>>>>>> 
>>>>>> On 04/12/2018 15:51, Jorn Vernee wrote:
>>>>>> 
>>>>>>> Maybe this concern is being raised prematurely. I think we can see how the current plans pans out and then look at this again if needed.
>>>>>>> 
>>>>>>> 
>>>>>> Yes, let's revisit this later. I have a couple of different ideas on how to get there, but it doesn't make a lot of sense to discuss them here, as it's probably too early - and I don't want to add to the confusion.
>>>>>> 
>>>>>> To recap (for Henry): let's have SystemABI::upcallStub return a Symbol and take it from there.
>>>>>> 
>>>>>> Thanks for your comments, as usual.
>>>>>> 
>>>>>> Maurizio
>>>>>> 
>>>>>> 
>>>>>>> Jorn
>>>>>>> 
>>>>>>> Jorn Vernee schreef op 2018-12-04 16:34:
>>>>>>> 
>>>>>>>> Maurizio Cimadamore schreef op 2018-12-04 16:07:
>>>>>>>> 
>>>>>>>>> On 04/12/2018 14:23, Jorn Vernee wrote:
>>>>>>>>> 
>>>>>>>>>> We have Scope::allocateCallback to replace step 2. and 3., but that requires an annotated functional interface object. What if a user just has a MethodHandle?
>>>>>>>>>> 
>>>>>>>>> Not sure I follow - a Callback is based on the idea that you _do have_
>>>>>>>>> a functional interface.
>>>>>>>>> 
>>>>>>>> The scenario I'm talking about still uses a functional interface
>>>>>>>> _type_. But the Scope::allocateCallback api expects _an instance_ of
>>>>>>>> such a functional interface type; So if you just have a MethodHandle
>>>>>>>> you would first have to turn it into an instance of this functional
>>>>>>>> interface type and then call Scope::allocateCallback. This adds a
>>>>>>>> level of indirection, and AFAIK there's no straight forward API for
>>>>>>>> wrapping a MethodHandle in a functional interface in the JDK
>>>>>>>> (MethodHandleProxies comes close but it relies on reflection, so it's
>>>>>>>> slow).
>>>>>>>> 
>>>>>>>> So what I'm talking about is that a user can use System::upcallStub to
>>>>>>>> make the MethodHandle callable from native, but if they have some
>>>>>>>> other method that accepts a Callback there still needs to be a way to
>>>>>>>> convert the value returned from upcallStub into a Callback.
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> I don't think it makes sense to mix the abstraction levels - the
>>>>>>>>> binder will represent all callbacks using Callback, so I don't expect
>>>>>>>>> a user of the high-level binder to mess with SystemABI interface to
>>>>>>>>> allocate stubs.
>>>>>>>>> 
>>>>>>>> I was thinking about the inverse; we have a user who is using the
>>>>>>>> low-level SystemABI, and wants to have the high-level addition of
>>>>>>>> Callback. This also makes sense where low-level code has to interact
>>>>>>>> with high-level code.
>>>>>>>> 
>>>>>>>> Jorn