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

[Maurizio Cimadamore]

On 23/07/2019 15:04, Jorn Vernee wrote:
> On 2019-07-23 15:54, Maurizio Cimadamore wrote:
>> On 23/07/2019 14:48, Jorn Vernee wrote:
>>> 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 :)
>>>
>> Yes - that's another way to skin the cat, which would work pretty well.
>>
>> My 1000 feet view is that having 'annotations' is likely to be
>> confusing in the user model ("excuse me, are they Java annotations?")
>> - so the more we can put boundaries around them the better.
>>
>>
>>> 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).
>>
>> Uhm - not sure about this; it depends what we want to achieve by
>> comparing the two descriptors. If that should be just a comparison of
>> the 'signatures' I'd say that's fine. Here you are appealing on a
>> deeper notion of comparison - which means "and the two functions shall
>> be executed in exactly the same way", which is a lot stronger.
>
> This is exactly what I'm looking for. e.g. we can create a cache of 
> FunctionDescriptor -> MethodHandle in the ABI implementations, and 
> just have to pass in a different function pointer for different 
> functions we want to bind.
>
> We can use a separate method for that as well, doesn't have to be 
> FunctionDescriptor::equals, but maybe the latter makes more sense.
>
>> Also, I as noted previously, I wouldn't let the varargs case drive the
>> design too much - all we're doing around varargs is a placeholder for
>> something better (a true Valist carrier).
>
> Maybe I am... I'm trying to use it as proof that these ABI dependent 
> distinctions do exist, and we might see more of them in the future. 
> But, to be fair, we can cross that bridge when we get to it.

Well, it seems to me that, regardless of what we do for equality, 
there's a way to do what you want to do with the current API.

But my current thinking is that what we wanna do in the internals is 
pretty different from what people will do outside.

Maurizio

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