[foreign-abi] [Rev 01] RFR: 8245988: Add a special VaList carrier

[Maurizio Cimadamore]

On Thu, 4 Jun 2020 10:15:00 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds a special VaList carrier to CSupport that can be used by C linker implementations to pass `va_list`
>> arguments to upcalls and downcalls.
>> Currently Windows VaList and SysV VaLists are implemented, but AArach64 not yet.
>> 
>> The API for the Reader and Builder might be a bit strange; the limitation to int, long, and double comes from the
>> standard argument promotions that are done in C when calling a variadic function. Since we do not know how to promote
>> any arbitrary value, at least for now the API restricts itself to accepting only types that would not be promoted in C,
>> leaving any needed conversion up to the user.  For reviewers; I moved CallArranger.TypeClass + classification code to
>> the top level, since it is now shared with the VaList implementations, so that's where the CallArranger diff comes
>> from.  Thanks, Jorn
>
> Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since
> the last revision:
>  - Add VaList benchmarks
>  - Finished testing VaList and added MemoryAddress <-> VaList converters
>  - - Added testing of structs passed on the stack.
>    - Added documentation.
>  - Merge Reader and VaList interfaces
>  - Copy by-value structs to maintain semantics
>  - Add support for a special VaList carrier to upcalls and downcalls

Looks very good; a nice dependable layer upon which to build vararg support for both upcalls and downcalls. Seems more
elegant than the ad-hoc support for variadic layout arguments we currently have.

I've added some minor comments, mostly stylistic and API-related.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CSupport.java line 76:

> 75:          */
> 76:         int readInt(MemoryLayout layout);
> 77:

Here some confusion might arise as to what `int` means. `int` means Java `int`.
As discussed offline, it would be better to use some hint e.g. `from` to indicate that we are converting *from* a Java
value to a native one. Also, `read` for some reason is reminiscent of I/O.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CSupport.java line 109:

> 108:         MemorySegment readStructOrUnion(MemoryLayout layout);
> 109:
> 110:         /**

These two do not seem consistent with the builder methods

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CSupport.java line 170:

> 169:              */
> 170:             Builder intArg(MemoryLayout layout, int value);
> 171:

Dual issue here - `int` again stands for Java `int`. Perthaps using `asInt` in the method name could help clarify.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVx64Linker.java line 94:

> 93:
> 94:     private static MethodHandle unxboxVaLists(MethodType type, MethodHandle handle) {
> 95:         for (int i = 0; i < type.parameterCount(); i++) {

type here - unxbox -> unbox

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVx64Linker.java line 116:

> 115:                handle = MethodHandles.filterArguments(handle, i, MH_boxVaList);
> 116:             }
> 117:         }

These looks a bit odd - in the sense that we have to hide valists from call arranger, but then add adaptation back in
to make clients happy. I guess it seems as if this could be less error prone if CallArranger would also be in charge of
dealing with valists.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/WinVaList.java line 231:

> 230:                     VarHandle writer = arg.varHandle();
> 231:                     writer.set(addr, arg.value);
> 232:                 }

>From a performance perspective, both this and the reader code above are suboptimal, in two dimensions:
* the VH used is not constant
* the type of the get/set call is not exact
No need to dive in premature optimization territory now, but perhaps worth keeping in mind.

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

Marked as reviewed by mcimadamore (Committer).

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