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

[Maurizio Cimadamore]

On 23/07/2019 11:49, Nick Gasson wrote:
> Hi Maurizio,
>
> Looks good, just a few issues I ran into when I tested on Arm.
>
> In CallingSequenceBuilder, this change:
>
>          if (returnsInMemory) {
> -           retInfo = makeArgument(MemoryLayout.ofAddress(64, ret), -1, "__retval");
> +           retInfo = makeArgument(MemoryLayouts.SysV.C_POINTER, -1, "__retval");
>              addArgumentBindings(retInfo, false);
>          }
>
> Causes a ClassCastException at runtime because we try to cast an
> x64.ArgumentClassImpl to an aarch64.ArgumentClassImpl. I think Windows
> will also have this problem. Should we introduce
> MemoryLayouts.C_POINTER, etc. which picks the appropriate ABI for the
> platform? (Just like we have in NativeTestHelper now.)

Whoops - forgot about that one.

We should do something similar to what NativeTestHelper does, but inside 
the CallingSequenceBuilder. I don't want a magic constant exposed.

>
> In NativeTestHelper.java:
>
>    51     public static final ValueLayout C_DOUBLE = pick(MemoryLayouts.SysV.C_DOUBLE, MemoryLayouts.WinABI.C_DOUBLE, MemoryLayouts.AArch64ABI.C_POINTER);
>    52     public static final ValueLayout C_POINTER = pick(MemoryLayouts.SysV.C_POINTER, MemoryLayouts.WinABI.C_POINTER, MemoryLayouts.AArch64ABI.C_DOUBLE);
>
> C_POINTER and C_DOUBLE are swapped for AArch64.
>
> I got a crash in TestDowncall calling this function:
>
> # C  [libTestDowncall.so+0xbd264]  f10_S_S_DDP+0x14
>
> It takes a struct with two doubles and a pointer. This should be passed
> on the stack, but CallingSequenceBuilderImpl was incorrectly classifying
> it as a "homogeneous floating-point aggregate" (struct with all floating
> point members of the same size which are passed in vector registers). I
> fixed it with this change:
>
> @@ -148,14 +148,21 @@ public class CallingSequenceBuilderImpl extends CallingSequenceBuilder {
>           if (!(baseType instanceof ValueLayout))
>               return false;
>
> -        boolean isIntegral = ((ArgumentClassImpl)Utils.getAnnotation(baseType, ArgumentClassImpl.ABI_CLASS)).isIntegral();
> -        if (isIntegral)
> -            return false;
> +        ArgumentClassImpl baseArgClass =
> +            (ArgumentClassImpl)Utils.getAnnotation(baseType, ArgumentClassImpl.ABI_CLASS);
> +        if (baseArgClass != ArgumentClassImpl.VECTOR)
> +           return false;
>
>           for (MemoryLayout elem : groupLayout.memberLayouts()) {
>               //Todo: need to strip names
>               if (!elem.equals(baseType))
>                   return false;
> +
> +            // TODO: should AbstractLayout::equals check this??
> +            ArgumentClassImpl argClass =
> +                (ArgumentClassImpl)Utils.getAnnotation(elem, ArgumentClassImpl.ABI_CLASS);
> +            if (argClass != baseArgClass)
> +                return false;
>           }
>
>           return true;
>
> But I wonder if AbstractLayout::equals should be checking the
> annotations for equality? The original code would have worked in that
> case.
Right, the patch changes the layout equality. I think that, in general 
we should only compare the layout contents for equality - annotations 
are more like side-channel info. So what you have is more robust.
>
> I think it would be good to port over the CallingSequenceTest unit tests
> from the foreign branch, they are useful for finding problems like
> this. I'm happy to do the work if you agree?
Sure - let's get this sorted out first and then let's port the test
>
> Thanks,
> Nick
>
>
> On 23/07/2019 01: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
>>
>>
>>