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

[Maurizio Cimadamore]

I'm fine leaving things where they are, for now.

Thanks!
Maurizio

On 11/01/2019 13:45, Jorn Vernee wrote:
> I've removed the sizeof methods.
>
> About the other 2 points. These are smells, but they were already 
> present, and I think the problem goes a deeper as well. For instance, 
> lots of places use NativeTypes.UINT64 to represent a "slot" of memory, 
> but that would probably not hold up on 32bit platforms. So maybe we 
> can fully figure out the re-stacking when a non-x64 port comes along 
> and we have a clearer picture about the exact requirements? I can add 
> FIXMEs for now.
>
> As a quick fix, we could move all the classes in the abi package, 
> except SystemABI and UpcallStubs, into the abi/x64 package, since 
> there are evidently dependencies on x64 specific stuff. And then when 
> the next port comes along figure things out at that point?
>
> Jorn
>
> Maurizio Cimadamore schreef op 2019-01-11 11:09:
>> 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 
>>>>>>