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

[Maurizio Cimadamore]

On Tue, 15 Sep 2020 16:53:38 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
>
> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments:
>   - remove C_BOOL
>   - CLinker javadoc changes
>   - rename CLinker::getSystemLinker to CLinker::getInstance()

Looks better -  a lot tighter than it ever was - still a couple of comments and a question to @PaulSandoz as to
whether, from an API perspective, it makes sense to have an interface with only one supported impl.

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

> 55:  * a failure to do so might result in linkage errors, given that linking requires additional classification
> 56:  * information to determine, for instance, how arguments should be loaded into registers during a
> 57:  * foreign function call.</p>

I think this paragraph is general enough that it would work very well on the top ForeignLinker interface (replace C
with any other language). I also wonder if we should add an example here - e.g. for `strlen` ?

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

> 142:      * The prefix of all ABI attribute names.
> 143:      */
> 144:     String ABI_ATTR_PREFIX = "abi/";

does this go here? Not sure what it buys to clients.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ForeignLinker.java line 35:

> 33:  * methods as a native function pointer (modelled as a {@link MemorySegment}).
> 34:  *
> 35:  * Instances of this interface can be obtained for instance by calling {@link CLinker#getSystemLinker()}

This comments should says something else - e.g. ```
Subclasses of this interface provide support for different linking strategies. For instance, the {@link CLinker}
interface defines a foreign linker which supports the C ABI.```

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

> 91:  * {@link #asVarArg(MemoryLayout)} is used to create the memory layouts for each parameter corresponding to a
> variadic 92:  * argument in a specialized function descriptor</p>
> 93:  */

There should be the usual comment about sealedness and immutability - see MemoryLayout

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

> 416:      * and any other type that fits into a {@code long}.
> 417:      */
> 418:     interface VaList extends Addressable, AutoCloseable {

Comment about sealedness and immutability here - probably for the Builder too

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

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