RFC: Refactoring SystemABI

[Maurizio Cimadamore]

I don't think this has to be a binary decision where either everything 
is shared or evrything is duplicated. The right answer will probably sit 
in some messy point in the middle, when some parts of the code are 
shared, while some are not. And we don't have to find the right balance 
at the first attempt.

But the new SystemABI interface gives us lots of option - in addition of 
being a truly credible API to interop with low level system capabilities 
in Java. It's an API I can stand for. How it's implemented under the 
hood is a different matter, and I can see us iterating a bunch of times 
before finding the right balance. The more reuse, of course, the less 
hidden bugs and easier to test; but if you dial all the way down to 
shared you are going to find other artifacts of the kind 'trying to fit 
a square peg in a round hole'.

The low level building blocks - such as CallingSequence and 
ShuffleRecipe should be shared, I believe, and so should the universal 
invoker/upcall code paths (as much as possible). But I think that when 
you get into varargs land, it doesn't make sense to expose varargs info 
to SystemV ABI since that ABI has no use for it. The varargs treatment 
for Windows ABI is very special and will likely have to be done with a 
separate varargs invoker, I believe (to do it properly).

Anyway, let's do one thing at a time; let's see if we can complete the 
work on SystemABI and push that back. At which point we'll have more 
options when it comes to add Windows support. But that is mostly an 
implementation problem that doesn't have much to do with the current 
effort from Henry, so let's try to keep this focused, for now. Once 
things are in place, we can play around with several patches and see 
which one we like the most in terms of code cleanliness, 
maintainability, etc. Ok?

Maurizio

On 24/11/2018 22:34, Jorn Vernee wrote:
> I'm having second thoughts on this again.
>
> It might be nice to have 2 copies for the different ABIs, but it will 
> be a lot of code duplication that has to be fixed later.
>
> I know I said to move CallingSequence + ShuffleRecipe::make to the 
> SysV impl package, but I think the changes that I have there are 
> portable any ways, so they could stay where they are as well. It's 
> also not just those 2 things, I also changed Argument, and 
> CallingSequenceBuilder to have a flag for if the Argument is being 
> passed as varargs. I think that's portable as well, but it also 
> touches the SysV code, since it now has to say whether an argument is 
> varargs or not. I'm thinking what the best choice there is, and if we 
> should keep that code shared or have separate copies, but right now 
> I'm leaning towards shared.
>
> The painful part for me is that any changes I make in shared code will 
> have to be maintained by me alone until all the Windows support is 
> done. Right now the binder + VM changes are done, and it's just some 
> jextract tests that are failing, which I think mostly has to do with 
> the tests setting the expectations for the SysV ABI, but the 
> underlying Clang API is following the Windows ABI. It would be nice if 
> I could merge the binder + VM changes before starting to fix those, or 
> at least part of it.
>
> I could make a patch, after the SystemABI refactoring is merged, that 
> does all the changes to the shared code which are portable, and then 
> merge that so that I will have the API I need to implement the Windows 
> changes, but I won't have to maintain it by myself.
>
> Does this sound OK?
>
> Jorn
>
> Jorn Vernee schreef op 2018-11-22 15:32:
>> Thinking some more about this;
>>
>> I think you can just move those classes to the SysV ABI package for
>> now, and then when I make the patch for Windows support I will create
>> a new abi.common package where I will put part of the
>> UniversalNativeInvoker (with abstract methods for boxing/unboxing) and
>> probably the entire UniversalUpcallHandler (I had to make changes
>> there, but I don't think they conflict with SysV).
>>
>> Then if somebody comes in and wants to add another port, they can copy
>> and paste one of the ABI impl folders (whichever one is closest to
>> their target ABI), and then make changes until the port is done. Then
>> for the patch remove the duplicated code by moving it into the
>> abi.common package. If needed they could even copy the common stuff to
>> make changes independently, the important thing is that the SystemABI
>> API points are as flexible as possible to allow for separate
>> implementations of upcall/downcall stuff.
>>
>> So for now, you can just move those classes (invokers, upcallhandlers
>> and CallingSequence + ShuffleRecipe::make) to the SysV impl package.
>>
>> Jorn
>>
>> Jorn Vernee schreef op 2018-11-22 13:53:
>>> Maurizio already had a lot of good points, the most important one
>>> being #5 in my opinion:
>>>
>>>> All the various XYZUpcallHandle/XYZInvokers/XYZSignatureShuffler, 
>>>> should live in the SysV ABI impl package (these last two points can 
>>>> be done in a final renaming step)
>>>
>>> The idea is that these classes do ABI specific work, and I've had to
>>> make changes to most of these (except of course the signature
>>> shufflers) to make them work on Windows. The biggest thing being the
>>> re-writing of boxing/unboxing code. The idea for now was to copy-paste
>>> these into the Windows ABI and then make independent changes, and if
>>> there is commonality, factor that out into a top-level class. So I
>>> think we might have a top-level package with AbtractInvoker and
>>> AbstractUpcallHandle at some point for ABIs to use as a template for
>>> their own invokers. And these top-level classes would be a good place
>>> to put debugging code as well.
>>>
>>> One point of commonality I can already predict is the downcall into
>>> the VM. For now we could reuse those native methods, and maybe they
>>> should be put into a top-level utility class. In a next refactoring
>>> step we could maybe separate the VM code as well, and also have
>>> separate native methods for the downcalls. I ran into very little
>>> friction on the native side, probably because the shuffle recipe stuff
>>> was very general-purpose already, and because in C++ we have compiler
>>> switches, so I'd prefer if the VM implementation code was left
>>> untouched in this refactoring round (this is currently the case). I'll
>>> have an easier time integrating these changes first.
>>>
>>> One other thing I think should be moved to the SysV impl package is
>>> CallingSequence. Since register allocation is different on windows
>>> I've had to visit that class many times to fix bad merges. For now
>>> it's easier to copy-paste and maintain a separate copy. AFAICT that
>>> also means moving the code in ShuffleRecipe::make into the SysV impl
>>> package.
>>>
>>> The abi packages are currently structured like `abi/SysV/x64` and I've
>>> added `abi/Microsoft/x64`, but I think this should be changed to:
>>> `abi/x64/SysV` and `abi/x64/Microsoft`. I think having the same
>>> bit-ness gives the ABIs more commonality then the same name.
>>>
>>> But, all-in-all this is looking great so far ������.
>>>
>>> Cheers,
>>> Jorn
>>>
>>> Henry Jen schreef op 2018-11-22 09:21:
>>>> Updated webrev[1]
>>>>
>>>> - Added NativeMethodType and use it in SystemABI. This also allow us
>>>> to have varargs downcall MH from SystemABI considering that different
>>>> platform may handle varargs differently.
>>>> - UpcallHandler is now named UpcallHandle, and is returned by
>>>> SystemABI for upcallStub.
>>>>
>>>> Cheers,
>>>> Henry
>>>>
>>>> [1] 
>>>> https://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.01/webrev/ 
>>>>
>>>>
>>>>> On Nov 16, 2018, at 2:05 PM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>>>>>
>>>>> Comments inline...
>>>>>
>>>>> Henry Jen schreef op 2018-11-16 18:50:
>>>>>>> On Nov 16, 2018, at 8:23 AM, Maurizio Cimadamore 
>>>>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>>>> Hi Jorn,
>>>>>>> what you propose is a possible refinement; I've been noodling 
>>>>>>> over this myself when writing the SystemABI interface in an 
>>>>>>> attempt to find a way to reduce the number of parameters in the 
>>>>>>> SystemABI calls, to make the API simpler. But I then decided to 
>>>>>>> strive for minimality, at least for now.
>>>>>>> That said, creating the "native" equivalent of MethodType is 
>>>>>>> something that makes total sense, and we should explore this 
>>>>>>> path further.
>>>>>> I think that’s Function + MethodType, not sure if we need an
>>>>>> abstraction on top of those two. Create another type to hold two
>>>>>> information seems an overkill for me.
>>>>>
>>>>> I dont't think MethodType is good enough. That would treat a 
>>>>> Pointer<MyStruct> as just a Pointer, since MethodType uses Class. 
>>>>> You can get the layout of the struct from the `Function`, but you 
>>>>> wouldn't know which Java carrier type you should actually use for it.
>>>>>
>>>>> But LayoutTypeImpl saves the pointee type if you create a pointer 
>>>>> type [1], so that retains the information.
>>>>>
>>>>>>> As for adding the 'binding' method directly in the 
>>>>>>> Library.Symbol method, I feel less sanguine about it; if you 
>>>>>>> have a method in the SystemABI interface like:
>>>>>>> MethodHandle downcallHandle(Symbol s, FunctionType ft)
>>>>>> I gave this a thought, I am leaning toward to keep it as is. Note 
>>>>>> that
>>>>>> SystemABI gave the final method handle which should have all the
>>>>>> arguments actually passed into the call, varargs should have been
>>>>>> expanded. So FunctonType for SystemABI don’t really give us extra
>>>>>> value except we need to compose such object from the argument 
>>>>>> binding.
>>>>>
>>>>> Right. NativeInvoker.of, which creates VarargsInvoker, is 
>>>>> currently being called in HeaderImplGenerator and 
>>>>> CallbackImplGenerator, and they pass on the generics information 
>>>>> by using j.l.r.Method. But if we want to move these calls into 
>>>>> SystemABI as well then I think we want another way to pass on that 
>>>>> information. Since not every binding operation will have a Method 
>>>>> for it, just relying on Method will not work for that, and we 
>>>>> can't rely on just MethodType for the same reason I gave above; it 
>>>>> loses the generics information.
>>>>>
>>>>> There are also some differences between ABIs when it comes to 
>>>>> varargs. For instance, the CallingSequence generation mechanism on 
>>>>> windows needs to know exactly which arguments are being passed as 
>>>>> varargs to determine which registers to use, but on SysV we don't 
>>>>> care about that. I'm thinking maybe the creation of VarargsInvoker 
>>>>> should be pushed behind SystemABI as well. Then windows can pass 
>>>>> on which arguments are varargs in an independent implementation 
>>>>> from the SysV ABI.
>>>>>
>>>>> With that, having varargs information being passed to SystemABI 
>>>>> also makes sense, since SystemABI needs to decided if it wants to 
>>>>> create a VarargsInvoker or not (and maybe which kind of 
>>>>> VarargsInvoker).
>>>>>
>>>>> Jorn
>>>>>
>>>>> [1] : 
>>>>> http://hg.openjdk.java.net/panama/dev/file/28b312ac55b5/src/java.base/share/classes/jdk/internal/foreign/memory/LayoutTypeImpl.java#l100
>>>>>
>>>>>> On the other hand, varargs method handle need to be composed at 
>>>>>> higher
>>>>>> level, where I think Function + MethodType or FunctionType makes 
>>>>>> more
>>>>>> sense. Should we have this in the SystemABI as a default method or
>>>>>> have it as an factory method in Symbol, I think I prefer Symbol.
>>>>>> Cheers,
>>>>>> Henry
>>>>>>> I guess that would achieve the same effects? That is, if we have 
>>>>>>> a FunctionType then I would use it to simplify all signatures in 
>>>>>>> the SystemABI interface, at which point creating a MH from a 
>>>>>>> symbol is pretty straightforward.
>>>>>>> Maurizio
>>>>>>> On 16/11/2018 16:50, Jorn Vernee wrote:
>>>>>>>> Hi,
>>>>>>>> I like the idea of moving away from the dependency on 
>>>>>>>> `j.l.r.Method`, I see that in a lot of places you've started 
>>>>>>>> using `LayoutType<?> ret, LayoutType<?>... args`. I really like 
>>>>>>>> this, and I think it could use it's own wrapper type. I had 
>>>>>>>> experimented earlier with a draft of this:
>>>>>>>> public class FunctionType {
>>>>>>>>    private final LayoutType<?> return;
>>>>>>>>    private final LayoutType<?>[] args;
>>>>>>>>    private final boolean isVararags;
>>>>>>>>    private FunctionType(LayoutType<?> return, LayoutType<?>[] 
>>>>>>>> args, boolean isVararags) {...}
>>>>>>>>    public static FunctionType make(LayoutType<?> ret, 
>>>>>>>> LayoutType<?>... args) {...}
>>>>>>>>    public static FunctionType makeVarargs(LayoutType<?> ret, 
>>>>>>>> LayoutType<?>... args) {...}
>>>>>>>>    public static FunctionType ofMethod(Method m, Function 
>>>>>>>> layout) {...}
>>>>>>>>    // public static FunctionType ofAnnotatedMethod(Method m) 
>>>>>>>> {...} // for later when we have per-method annotations
>>>>>>>>    public LayoutType<?> returnType() {...}
>>>>>>>>    public LayoutType<?>[] paramTypes() {...}
>>>>>>>>    public boolean isVarargs() {...}
>>>>>>>>    public MethodType toMethodType() {...}
>>>>>>>>    public Function toFunction(){...}
>>>>>>>> }
>>>>>>>> This could maybe later also be used as a way to bind individual 
>>>>>>>> symbols without having to create an interface to bind to:
>>>>>>>> class Library.Symbol {
>>>>>>>>    ...
>>>>>>>>    public MethodHandle bindAsFunction(FunctionType type) {...}
>>>>>>>> }
>>>>>>>> This doesn't work when using `Class`, since you lose generic 
>>>>>>>> information; `Pointer<Byte>` -> `Pointer`, but `LayoutTypeImpl` 
>>>>>>>> saves this information and makes it accessible through 
>>>>>>>> `pointeeType()`, and the API for creating them is currently 
>>>>>>>> really nice; `LayoutType<?> lt = NativeTypes.UINT8.pointer();` 
>>>>>>>> (similar with Array as well).
>>>>>>>> What do you think?
>>>>>>>> Jorn
>>>>>>>> Maurizio Cimadamore schreef op 2018-11-16 11:06:
>>>>>>>>> Hi Henry,
>>>>>>>>> the work looks like a good initial step. But I think you 
>>>>>>>>> didn't go all
>>>>>>>>> the way through with it. That is, the binder code 
>>>>>>>>> (HeaderImplGenerator
>>>>>>>>> and ScopeImpl) still rely on UpcallHandler/NativeInvoker. I 
>>>>>>>>> think it
>>>>>>>>> would be nice if we could completely decouple these things. 
>>>>>>>>> After all,
>>>>>>>>> everything that HeaderImplGenerator needs is a way to get a 
>>>>>>>>> method
>>>>>>>>> handle for a given downcall, and that can be done directly 
>>>>>>>>> through
>>>>>>>>> SystemABI, now.
>>>>>>>>> For upcalls its a bit harder, given that for an upcall there's
>>>>>>>>> currently some info associated to it:
>>>>>>>>> * receiver object
>>>>>>>>> * address of the generated stub
>>>>>>>>> * target method handle
>>>>>>>>> plus a way to 'free' the stub generated by the VM.
>>>>>>>>> I think the best way to get there is to create an intermediate
>>>>>>>>> abstraction that holds these info (plus a free() method), call it
>>>>>>>>> 'UpcallHandle' (removed the final 'R') and then let the binder 
>>>>>>>>> program
>>>>>>>>> against this. Then, a Callback object is essentially an 
>>>>>>>>> UpcallHandle +
>>>>>>>>> a Java carrier (functional interface) + a scope.
>>>>>>>>> But UpcallHandler as it is now, is not a mere abstraction 
>>>>>>>>> describing
>>>>>>>>> an upcall - it also embodies the strategy by which the upcall is
>>>>>>>>> performed (e.g. direct vs. universal) in the same way a 
>>>>>>>>> NativeInvoker
>>>>>>>>> does. So I think that moving forward we would probably want to 
>>>>>>>>> just
>>>>>>>>> have SystemABI select the right NativeInvoker/UpcallHandler to 
>>>>>>>>> do the
>>>>>>>>> job - and these things don't have to be superclasses, or induce a
>>>>>>>>> hierarchy of any kind - e.g. it would be ok for a 
>>>>>>>>> DirectInvoker to be
>>>>>>>>> completely unrelated from a UniversalInvoker. As long as the 
>>>>>>>>> rest of
>>>>>>>>> the binder only interacts with SystemABI, that's ok.
>>>>>>>>> Maurizio
>>>>>>>>> On 15/11/2018 22:44, Henry Jen wrote:
>>>>>>>>>> Hi,
>>>>>>>>>> Please have a look at the webrev[1] refactor the SystemABI 
>>>>>>>>>> APIs based on Mauricio’s proposal. SystemABI is suppose to 
>>>>>>>>>> encapsulate platform-specific details, this patch make 
>>>>>>>>>> NativeInvoker/UpcallHander and CalllingSequence to be 
>>>>>>>>>> implementation details, and thus can further be 
>>>>>>>>>> simplified/removed if we need to.
>>>>>>>>>> The binder suppose to only interact with SystemABI, that’s 
>>>>>>>>>> not completed yet in this webrev, but follow up work should 
>>>>>>>>>> get us there.
>>>>>>>>>> Concept of CallingConvention is introduced as an abstraction 
>>>>>>>>>> for now and is currently simply support default.
>>>>>>>>>> Anyway, it’s just first step, and I would like to see if it 
>>>>>>>>>> make sense to us.
>>>>>>>>>> Cheers,
>>>>>>>>>> Henry
>>>>>>>>>> [1] 
>>>>>>>>>> http://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev/