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

[Jorn Vernee]

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