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

[Jorn Vernee]

Hi Maurizio,

Thanks for the review, comments inline...

Maurizio Cimadamore schreef op 2019-01-10 18:15:
> 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.

Ok, I thought we would be going the other direction, so I did not spend 
much time yet on trying to de-duplicate code. I tried to touch the SysV 
and shared implementation as little as possible to keep maintenance 
easier, with the intent of de-duplicating code in a later patch if 
desirable.

> * 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.

The BOXER is also used by UniversalUpcallHandler, we could go with sub 
classing for both that and UniversalNativeInvoker, and they would 
probably delegate to a common implementation.

> * 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.

Sounds good.

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

Agreed.

> * 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.

Agreed.

> * 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.

- ArgumentInfo/StorageCalculator keep only one counter for the number of 
registers on Windows while SysV needs 2.
- StorageCalculator::addBindings has a special provision for varargs:

     if(width == 8 && storage.getStorageClass() == 
StorageClass.VECTOR_ARGUMENT_REGISTER && varargs.contains(arg)) {
         Storage extraStorage = new 
Storage(StorageClass.INTEGER_ARGUMENT_REGISTER, nRegs, 
Constants.INTEGER_REGISTER_SIZE);
         
bindings[StorageClass.INTEGER_ARGUMENT_REGISTER.ordinal()].add(new 
ArgumentBinding(extraStorage, arg, i * 8));

         if (DEBUG) {
             System.out.println("Argument " + arg.getName() + " will be 
passed in register " + StorageNames.getStorageName(extraStorage));
         }
     }

I don't think those 2 can just be replaced by the SysV implementation, 
but there's probably some opportunity to de-dupe code in other places.

> - StandardCall::arrangeCall and CallingSequenceBuilderImpl::build are
> also identical

Agree there. This code was moved from AbstractSystemABI to these classes 
when it got nuked.

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

Same here, maybe we should bring back AbstractSystemABI? Or otherwise 
create an ABI utility class?

Jorn

> That's all I have for now.
> 
> Cheers
> Maurizio
> 
> https://mail.openjdk.java.net/pipermail/panama-dev/2019-January/003680.html