[foreign-abi] RFR: 8248560: Specify the behaviour of the ForeignLinker returned by CSupport::getSystemLinker

[Maurizio Cimadamore]

On Tue, 15 Sep 2020 14:36:08 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Hi,
> 
> This patch creates a new class called CLinker, which extends ForeignLinker. The new class documents how linking works
> in the class's javadoc. This is done in a separate class since the specification is only relevant to linkers that
> support the C ABI.  With the new class there was no more need to have CSupport, so the contents of CSupport has all
> been move to CLinker.
> After some offline discussion with Maurizio, we realized that it doesn't make much sense to expose platform specific
> layout constants, since it is not possible to combine them with a platform specific linker any way, so for a particular
> platform, there is only one set of layouts that can be used, namely the ones that are dynamically picked based on the
> current platform. For that reason, all platform specific layouts have instead been moved to an internal PlatformLayouts
> class. In order for downstream APIs (like jextract) to be able to filter out the ABI attributes from layouts, I've
> added a String constant to CLinker that is used as a name prefix for all the ABI attribute names, so clients can for
> instance filter attribute names using `.startsWith(ABI_ATTR_NAME)`.  The ForeignLinker::name method has been dropped in
> favor of an internal enum.  The rest of the patch is just mechanical CSupport -> CLinker renames.  Thanks, Jorn

Added some initial API comments - impl comments to follow soon

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 99:

> 97:      * {@code permit}, {@code warn} or {@code debug} (the default value is set to {@code deny}).
> 98:      */
> 99:     static CLinker getSystemLinker() {

Should be `getSystemLinker` here? Also, the string passed to the restricted check doesn't match the name of the method

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 48:

> 46:  *
> 47:  * <p>There are two components that go into linking a foreign function: a method type, and
> 48:  * a function descriptor. The method type, consisting of a set of 'carrier' types constitutes

I'd suggest doing `<em>` around `carrier`, instead of using `'`

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 49:

> 47:  * <p>There are two components that go into linking a foreign function: a method type, and
> 48:  * a function descriptor. The method type, consisting of a set of 'carrier' types constitutes
> 49:  * the Java side of a call to a foreign function, while the function descriptor constitutes

I'd replace "constitutes the Java side of the call" with something more explicit like "...set of carrier types, which,
together, specify the Java signature which clients must adhere to when e.g. calling the underlying foreign function."

Similarly "constitutes the native side of the call"; I'd replace that with "the function descriptor contains a set of
memory layouts (link) which, together, uniquely specify the foreign function signature and classification information
(via custom layout attributes), so that linking can take place."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 52:

> 50:  * the native side of the call. Memory layout attributes are used in the function descriptor
> 51:  * to attach ABI classification meta-data to memory layouts, which are required for linking.
> 52:  * Clients of this API should use the prepared memory layout constants found in this interface

Clients of this API should build function descriptors using the predefined memory layout constants (based on a subset
of the built-in types provided by the C language), found in this interface; a failure to do so might result in linkage
errors, given that this linker requires additional classification information to determine e.g. how arguments should be
shuffled during a foreign function call.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 68:

> 66:  *   (see {@link Integer#SIZE} and similar fields in other primitive wrapper classes).</li>
> 67:  *
> 68:  *   <li>If the carrier type is {@code MemoryAddress} or {@code VaList}, then the corresponding memory layout must
> be a

I would split this into two, one for pointers and one for VaList since the carrier and layout is different

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 80:

> 78:  * list or with an empty formal parameter list, are not supported directly. It is not possible to create a method
> handle 79:  * that takes a variable number of arguments, and neither is it possible to create an upcall stub wrapping a
> method 80:  * handle that accepts a variable number of arguments. However, for down calls only, it is possible to link
> a native

down calls or downcall?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 81:

> 79:  * that takes a variable number of arguments, and neither is it possible to create an upcall stub wrapping a method
> 80:  * handle that accepts a variable number of arguments. However, for down calls only, it is possible to link a native
> 81:  * variadic function by using a 'specialized' method type and function descriptor: for each argument that is to be

specialized should be in `<em>`

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 83:

> 81:  * variadic function by using a 'specialized' method type and function descriptor: for each argument that is to be
> 82:  * passed as a variadic argument, an explicit carrier type and memory layout must be present in the method type and
> 83:  * function descriptor when linking the function. Furthermore, the memory layouts of variadic arguments must

Furthermore, as memory layouts corresponding to variadic arguments in a function descriptor must contain additional
classification information, it is required that the function {@link #asVarArg(MemoryLayout)} is called on each
parameter layout that is used to compose a specialized variadic function descriptor.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 140:

> 138:
> 139:     /**
> 140:      * The {@code _Bool} native type.

I'd rewrite these comments as `The layout for the {@code XYZ} C type`

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

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