[foreign] RFR 8216485: Binder changes are needed to support paname foreign API on Windows

[Maurizio Cimadamore]

Continuing from [1].

This is a more in depth review of the binder changes.


Overall the changes look good, and what I see is more or less what we 
have discussed over the past few weeks. As a general comment/overarching 
theme, I think we should try, as much as possible, to reuse code where 
possible, rather then duplicating it.

* I see different idioms at play; for instance, for VarargsInvoker you 
opted for separate subclasses in the various ABI impl. But for 
UniversalInvoker you went in with a shared implementation using an 
helper 'boxer' class. I think should pick a reuse approach (subclassing 
vs. helpers) and stick with it uniformly. I slightly lean for subclassing.

* CallingSequence is still hardwired to the 
Constants::ARGUMENT_STORAGE_CLASSES in SysV. Not a big deal now (as the 
two constants classes define enums in a similar way), but I think it's 
an accident waiting to happen. I think we should eliminate 
ARGUMENT_STORAGE_CLASSES and RETURN_STORAGE_CLASSES from Constants, and 
put them in StorageClasses (which is a single source of truth), and then 
make sure that CallingSequence does not refer to anything inside the 
specialized 'abi' packages.

* I don't think I see a reason for duplicating ArgumentClass - in fact 
they are the same file

* For StorageNames, it is somewhat sad that we have to replicate bigger 
logic just to define different register arrays. Maybe something that 
could work is to define registers more explicitly in the abi/x64 package

enum Register {
     RAX("rax", StorageClass.INTEGER),
     ...
}

At which point maybe Constants can also have some EnumSet of 
integer/vector (and, if needed x87) registers.

* StandardCall vs. CallingSequenceBuilderImpl - this is tough; after 
spending some time staring at the code and the differences, it seems to 
me that:

- ArgumentInfo can be reused across both (the SysV seems more general)
- StorageCalculator can be reused across both (again the SysV is more 
general)
- StandardCall::arrangeCall and CallingSequenceBuilderImpl::build are 
also identical

So, the variable parts are/seem to be: examineArgument and classifyType 
(which is nice, because these are just simple function from Layout to 
something else), which have ABI dependent logic. Again, we have to 
decide whether to go with a subclass or helper approach for code reuse.

* The alignment routines in CallingSequenceBuilderImpl seem identical to 
the ones in SysVABI, so perhaps again these could be shared somewhere.


That's all I have for now.

Cheers
Maurizio

https://mail.openjdk.java.net/pipermail/panama-dev/2019-January/003680.html