[foreign-memaccess+abi] RFR: 8292047: Consider ways to add linkage parameters to downcall handles [v2]

[Maurizio Cimadamore]

On Thu, 22 Sep 2022 14:56:26 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch implements a linker option interface, that can be using to indicate additional linkage requirements to the linker.
>> 
>> The variadic function descriptor support is dropped in favor of using a linker option to indicate the first variadic argument index. I think the switch turned out to be straight forward for users of this feature as well, though a bit more verbose.
>> 
>> I also experimented with putting linker options on FunctionDescriptor directly, but it increases the complexity of the combinators of FunctionDescriptor. For instance, if an argument layout is inserted somewhere, should the variadic index option also be updated? (a similar issue arises if we support other linker options that include parameter list indexes). As a result of that, it seemed simpler to just move the linker options as an additional argument to a link request, since at that point we know  the descriptor won't change anymore.
>> 
>> Another design decision is to only allow a single instance of a linker option of a certain type (and they are stored internally in a `Map<Class<?>, Option>` as well). This makes it easier to determine whether a combination of linker options is valid.
>> 
>> Other option kinds that could be added in the future are listed in the JBS issue, but not part of this patch.
>
> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
> 
>   characteristic -> option

I think this looks good. As you say, it's a quite straightforward change, but one that gives us a lot of extra flexibility when linking downcalls/upcalls.

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

> 218:      * @throws IllegalArgumentException if the provided function descriptor is not supported by this linker.
> 219:      *                                  or if the symbol is {@link MemorySegment#NULL}
> 220:      * @throws IllegalArgumentException if an invalid combination of linker options is given

Suggestion:

     * @throws IllegalArgumentException if an invalid combination of linker options is given.

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

> 243:      * from the provided function descriptor.
> 244:      * @throws IllegalArgumentException if the provided function descriptor is not supported by this linker.
> 245:      * @throws IllegalArgumentException if an invalid combination of linker options is given

Suggestion:

     * @throws IllegalArgumentException if an invalid combination of linker options is given.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 148:

> 146:             Class<?> carrier = mt.parameterType(i);
> 147:             MemoryLayout layout = cDesc.argumentLayouts().get(i);
> 148:             if (varArgsOnStack() && options.isVarargsIndex(i)) {

nice!

test/jdk/java/foreign/TestLinker.java line 42:

> 40:     @Test
> 41:     public void testLinkerOptionsCache() {
> 42:         Linker linker = Linker.nativeLinker();

Good test

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

Marked as reviewed by mcimadamore (Committer).

PR: https://git.openjdk.org/panama-foreign/pull/734