[foreign-abi] RFR: JDK-8242127: reorganize ABI-dependent layout constants

[Jorn Vernee]

On Fri, 3 Apr 2020 18:26:40 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> There are two main problems with the way in which ABI-dependent constants are dealt with:
> 
> * the SystemABI.Type enum abstraction is rarely used - code just prefers to go with layouts constants in MemoryLayouts
> * the current enum doesn't scale to model platform-dependent ABI types, so it just has to work with the set of types that
>   are supported in all platforms
> 
> This patch rearranges things a bit. First, it removes the SystemABI.Type abstraction, and associated
> SystemABI::layoutFor method. Then, it moves all ABI layout constants from MemoryLayouts to SystemABI. Instead of using
> the enum value as a classification value for the ABI, each ABI constant holder defines its own classification
> constants. So there is a SystemABI.Win64.ArgumentClass, and a SystemABI.SysV.ArgumentClass and so on and so forth. A
> notable fact is that the set of classification values that are required by the ABI is much, much smaller than the set
> of "interesting" C types.  I then adjusted the implementation code not to no longer depend on the SystemABI.Type enum;
> while doing so I realized (suggestion from Jorn) that the public ArgumentClass enums would actually, both in Windows
> and AArch be enough to also implement the internal classification system. On SysV, since the classification is quite
> convoluted, there is a number of 'synthetic' classification classes that are only introduced when classifying struct
> (e.g. SSE_UP) which would be bad to expose outside - but we need these classes to keep the classification algorithm
> going. So, I moved ArgumentClassImpl under the sysv folder and kept the code as it was.  The resulting code is, I think
> more direct, and more scalable. We can keep adding layouts to SystemABI - both platform-independent and
> platform-dependent ones - they are just different sets of constants.

Looks good. I left a few comments inline.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SystemABI.java line 305:

> 304:             INTEGER,
> 305:             SSE,
> 306:             POINTER;

This could be called `FLOAT`, just like in the enum used in the Windows CallArranger. A little more descriptive I think.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java line 518:

> 517:     private static List<ArgumentClassImpl> classifyStructType(GroupLayout type) {
> 518:         if (type.attribute(SystemABI.SysV.CLASS_ATTRIBUTE_NAME)
> 519:                 .filter(ArgumentClassImpl.COMPLEX_X87::equals)

Dropping the cast and call to argumentClassFor looks incorrect to me. The value returned from `attribute` is from the
SystemABI.SysV.ArgumentClass enum, but it's being compared to a value from ArgumentClassImpl. Seems that would always
return false.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SystemABI.java line 149:

> 148:      ValueLayout C_DOUBLE = Utils.pick(SysV.C_DOUBLE, Win64.C_DOUBLE, AArch64.C_DOUBLE);
> 149:
> 150:     /**

Pre-existing, but handy to address now: `C_LONGDOUBLE` is missing here. It could be added. All 3 ABIs define the layout
and it's a standard data type.

-------------

Marked as reviewed by jvernee (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/97