[foreign-memaccess+abi] RFR: 8308293: A linker should expose the layouts it supports

[Jorn Vernee]

On Thu, 15 Jun 2023 10:27:53 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This is another stab at https://github.com/openjdk/jdk/pull/14037
> 
> I believe, after some offline discussion, that we have found a more satisfying solution to the problem of JAVA_CHAR being exposed. Jorn suggested that linkers should also provide mappings for JNI types such as `jint`, `jshort` and such (which are aliases for our layout constants anyway). I think that's a great way to bring `JAVA_CHAR` back into the fold.
> 
> For now, I decided not to specify support for JNI canonical layouts (but I could do so, if that's preferred). I think the highest priority is to provide some stable mappings for C builtin types (+ `size_t`) as that's what 99% of developers will be struggling with.
> 
> API-wise, we just expose a map. In the preovious PR there were questions as to whether the map should be split into two methods. In general I see the following options:
> 
> 1. Just expose a map (that's the primitive, other things can derived from it)
> 2. Expose a map, plus a method to get a canonical layout from a type name (that's the `Charset` approach, which has both `availableCharsets` *and* `forName`)
> 3. Expose a method to get canonical layout from name, plus a method that returns the set of supported canonical layout names
> 
> My (not so strong) preference would be for either (1) or (2).

src/java.base/share/classes/java/lang/foreign/Linker.java line 61:

> 59:  * via the generation of {@linkplain #upcallStub(MethodHandle, FunctionDescriptor, Arena, Option...) upcall stubs}.</li>
> 60:  * </ul>
> 61:  * A linker provides a way to lookup up the <em>canonical layouts</em> associated with the data types used by the ABI.

Suggestion:

 * A linker provides a way to look up the <em>canonical layouts</em> associated with the data types used by the ABI.

src/java.base/share/classes/java/lang/foreign/Linker.java line 65:

> 63:  * type. On 64-bit platforms, this canonical layout might be equal to {@link ValueLayout#JAVA_LONG}. The canonical
> 64:  * layouts supported by a linker are exposed via the {@link #canonicalLayouts()} method, which returns a map from
> 65:  * ABI type names to canonical layouts.

Not sure what "ABI type names" means here exactly (what is an "ABI type"?). Maybe just stick with "type names"?

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 142:

> 140:     @Override
> 141:     public Map<String, MemoryLayout> canonicalLayouts() {
> 142:         return CANONICAL_LAYOUTS_MAP;

I would prefer each linker to define it's own `static final` map, and implement canonicalLayouts() individually, rather than having a bunch of selection logic here in `AbstractLinker`. Since the mappings are ABI specific, and the linkers implement the ABI specific code, I think that is where this belongs.

Since most of the layouts are the same, I suggest adding a common factory method to SharedUtils with parameters for the layouts that change. (In practice we only use a single ABI per process, so we should only create the map once, and C2 should be able to fold away the virtual call).

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 283:

> 281:     private static final Map<String, MemoryLayout> CANONICAL_LAYOUTS_MAP = Map.ofEntries(
> 282:             // specified canonical layouts
> 283:             Map.entry("bool", ValueLayout.JAVA_BOOLEAN),

`bool` is just and alias for `_Bool`, should we define the latter as well?

What about `signed`/`unsiged` variants, or `short int`, `long int`, and `long long int`? (I'm looking at this table on wikipedia: https://en.wikipedia.org/wiki/C_data_types)

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 288:

> 286:             Map.entry("int", ValueLayout.JAVA_INT),
> 287:             Map.entry("float", ValueLayout.JAVA_FLOAT),
> 288:             Map.entry("long", CABI.current() == CABI.WIN_64 ? ValueLayout.JAVA_INT : ValueLayout.JAVA_LONG),

This also needs to cover Windows/AArch64.

Also, I don't think this will work for the fallback linker. In that case I think we need to get the native types from libffi, and then look at the size and alignment to determine the corresponding layout.

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

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/839#discussion_r1231091047
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/839#discussion_r1231093206
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/839#discussion_r1231080535
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/839#discussion_r1231081929
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/839#discussion_r1231080407