[foreign-abi] RFR 8228486: Add ABI-specific layout constants

[Jorn Vernee]

On 2019-07-23 15:40, Jorn Vernee wrote:
> Ok, this is also what we talked about a bit before.
> 
> Like you said in your other email; to support varargs on Windows we'd
> need to add some `MemoryLayout asVarArg(MemoryLayout)` method on
> SystemABI that does nothing for the other 2 ABIs, there are other
> points where the ABIs might diverge that are not naturally expressible
> as MemoryLayout constants as well (though we could add a
> Windows.Varargs version of all the constants as well). It seems that
> exposing annotations so that users can add ABI specific annotation to
> an argument layout is a bit cleaner.

Though, actually, we could also put that method in the 
MemoryLayouts.WinABI class and that would also be pretty clean :)

Jorn

> This also brings up the discussion of needing to account for
> annotations when checking for layout equality. I think at least for
> checking FunctionDescriptor equality we should take annotations into
> account as well. Then we can e.g. distinguish a function that takes a
> 64 bit float value normally, from one that takes a 64 bit float as a
> vararg on Windows. (similarly with int vs float values perhaps).
> 
> Jorn
> 
> On 2019-07-23 15:27, Maurizio Cimadamore wrote:
>> On 23/07/2019 14:20, Jorn Vernee wrote:
>>> Also, why aren't annotations exposed at the MemoryLayout level?
>> 
>> I wanted to experiment if we could get away w/o exposing them in the
>> API - but having them internally as a more stable basis for
>> implementing things like Layout names.
>> 
>> I'd like first to get a sense about how people will be using
>> annotations. Layout names are useful and referenced by the 'path' API;
>> general annotations don't have that kind of visibility in the API, so
>> there's a chance they are just for us/jextract.
>> 
>> Maurizio
>> 
>>> 
>>> Jorn
>>> 
>>> On 2019-07-23 15:07, Jorn Vernee wrote:
>>>> The tests pass on Windows. Overall the patch looks good
>>>> 
>>>> - in TestUpcall you seem to be using the SysV.C_POINTER constant
>>>> instead of the magic one.
>>>> 
>>>> - Do we need to distinguish between POINTER and INTEGER class?
>>>> Shouldn't this just be about in which register it's passed? It looks
>>>> like the isPointer predicate is only used in the tests in order to
>>>> construct dummy arguments, is there another way around this? (e.g.
>>>> checking that the carrier is a MemoryAddress).
>>>> 
>>>> Some things that I can look into:
>>>> 
>>>> * We can mark vararg arguments with a layout annotation on Windows 
>>>> to
>>>> be able to support vargargs on Windows as well.
>>>> 
>>>> * I think some of the code in Window's CallingSequenceBuilderImpl
>>>> could be removed at this point (e.g. the x87 stuff), since it's not
>>>> supported by the Windows C ABI any ways.
>>>> 
>>>> Jorn
>>>> 
>>>> On 2019-07-22 19:23, Maurizio Cimadamore wrote:
>>>>> Hi,
>>>>> the recent push for JDK-8228447, as expected, broke the foreign-abi
>>>>> branch, since that branch depended on the ValueLayout::isIntegral
>>>>> accessor.
>>>>> 
>>>>> This patch replaces that logic with the custom layout annotation 
>>>>> logic
>>>>> discussed in [1]:
>>>>> 
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8228486/
>>>>> 
>>>>> This patch makes the following changes:
>>>>> 
>>>>> * introduces three set of annotated layout constants, one for 
>>>>> supported ABI
>>>>> * the annotations point directly at the ABI-specific argument 
>>>>> classes
>>>>> * I've consolidated ArgumentClass a bit, so that now there's a 
>>>>> common,
>>>>> ABI-agnostic super-interface with some general predicates
>>>>> * AddressLayout has been removed (we can just look for 
>>>>> argumentClass::isPointer)
>>>>> * the tests have been tweaked to use the new ABI-specific 
>>>>> constants;
>>>>> since the test has to work across platforms, all ABI test use a new
>>>>> interface which imports the 'right' set of constants
>>>>> 
>>>>> While the patch might be a bit raw (I've only tested on Linux x64), 
>>>>> I
>>>>> like where this is going.
>>>>> 
>>>>> Maurizio