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

[Maurizio Cimadamore]

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