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

[Nick Gasson]

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.)

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.

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?

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