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

[Maurizio Cimadamore]

Thanks,
I think I'm still somewhat not 100% convinced this is the right 
stacking, but it's probably better to go ahead with this, and then see 
if we can simplify things a bit. All tests pass here, so no issues 
there. Some minor comments below:

* One thing with the new SharedConstants is that I noted that (as you 
also write) there is a dependency from UniversalUpcallHandler (which 
lives in abi) to SharedConstants (which lives in abi/x64). So we have a 
dependency from a less specific abi construct to a more specific; and 
that's probably a bad smell.

* The sizeof methods in CallingSequenceBuilder are not used and can be 
removed

* ArgumentClass is in abi/x64, but StorageClass isn't. Not sure, in 
general, in how do we differentiate between the two cases; in principle, 
anything that speaks about x87 should probably live under x64, but in 
reality, the shuffle recipes we generate rely on the fact that we have 
integer/vector/stack arguments and integer/vector/x87 returns...

Maurizio


On 10/01/2019 22:58, Jorn Vernee wrote:
> Followup on this.
>
> CallingSequenceBuilderImpl and StandardCall have more difference than 
> I realized. the arrangeCall method for the latter just lets you pass a 
> NaitveMethodType and the argument layouts are grabbed from that. But 
> on Windows we also have to differentiate between varargs and 
> non-varargs, so the SysV strategy doesn't work for both. I think this 
> is still solvable, of course, but I'd rather save that for a later 
> patch if that's okay with you. I have moved the alignment methods into 
> an abstract base class called CallingSequenceBuilder. The StorageNames 
> thing was only used by the CallingSequenceBuilder implementations, so 
> I've moved that logic into there as well.
>
> I've moved ArgumentClass to the abi/x64 package (Same for it's test), 
> and also created a SharedConstants class with some of the x86_64 
> constant values which are also used by UniversalUpcallHandler (this 
> was also referencing sysv/Constants before, which I changed to package 
> access to avoid that in the future).
>
> I've replaced the UniversalBoxer with sub classes of 
> UniversalNativeInvoker and UniversalUpcallHandler.
>
> I've changed microsoft -> windows for abi/x64/microsoft and 
> Microsoftx64ABI.
>
> updated webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/windows/webrev.04/
>
> Jorn
>
> Maurizio Cimadamore schreef op 2019-01-10 19:34:
>> On 10/01/2019 17:58, Jorn Vernee wrote:
>>> 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.
>>
>> Yes, I understand. We can also push this as is and work on incremental
>> improvements, but I'd like to unify code paths as possible, as I think
>> maintaining two completely separated code-bases will be trickier in
>> the long run. That said, I like that, at least for prototyping, the
>> new ABI design allows for slotting in new stuff quite easily.
>>
>> <snip>
>>
>>> - 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:
>> I might have gone too far here...
>>>
>>>
>>>> * 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?
>>
>> Note that the alignment function is ultimately only used by the
>> CallBuilder. So, if we go for subclassing, I'd say put this in
>> CallingSequenceBuilder - and then have the ABI-dependent details in
>> the Windows/SysV impls.
>>
>>
>> Also, I forgot an important point: please replace "microsoft" with
>> "windows", as that's what's used elsewhere (e.g. in hotspot sources).
>>
>>
>> Maurizio
>>
>>
>>>
>>> Jorn
>>>
>>>> That's all I have for now.
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>> https://mail.openjdk.java.net/pipermail/panama-dev/2019-January/003680.html 
>>>>