RFR: 7903585: Revisit variadic support

[Maurizio Cimadamore]

On Fri, 1 Dec 2023 13:34:03 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Currently, we only support var args passed to a variadic function through an `Object[]`. From that, we infer the layouts of the arguments, and then link a variadic method handle on the fly.
> 
> This is problematic because: 1. there's an ambiguity between structs and pointers, since they both use `MemorySegment` as a carrier. 2. from a performance perspective, re-linking the method handle every time is somewhat costly.
> 
> The approach proposed by this patch is to add a variadic invoker factory method, which can, given a list of layouts, be used to create an instance of a variadic invoker (interface), that embeds a downcall handle specialized to a particular type of the variadic function. The user specifies the layout explicitly, so there is no ambiguity (1), and the user can cache the returned invoker and re-use it instead of re-linking the function on every invocation (2).
> 
> The current layout inferring method can then be re-implemented on top of this factory.
> 
> Question: I'm currently generating the doc comment on both the inferring method and the invoker factory. Is this good? Should it be on one or the other? Should it be on the invoker interface as well?
> 
> ---
> 
> There were some issues I ran into while developing this patch, that I fixed:
> - We had some methods in the tests that take no arguments, but were declared without a prototype, leading to them getting translated as variadic functions in the bindings. I've change those parameter lists to `(void)` (this popped out when I only had the invoker factory in an earlier iteration)
> - I noticed that jtreg was sometimes not recompiling a test class in the generator tests after the bindings changed, leading to weird NoSuchMethodErrors. I've fixed this by adding an explicit `@build` step to the tests.
> - I've added `-retain:fail,error` to the jtreg arguments, so that the generated sources for a failing test are not deleted immediately, and can be manually inspected after a test failure.
> - I noticed we weren't generating a layout for `C_BOOL`, because we don't treat that type as being supported. I think this is a latent issue. I've added Bool as a supported type to fix this.
> 
> Also: I've ret-conned Test8244959 and renamed it to TestPrintf. And added new cases for the invoker.

Marked as reviewed by mcimadamore (Committer).

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 176:

> 174:             String paramExprs = paramExprs(declType, finalParamNames, isVarArg);
> 175:             appendLines(STR."""
> 176:                 public interface \{invokerName} {

question: if we used a record here, could we perhaps get rid of the holder class (since record fields are trusted) ? We don't expect this interface to be created by the user, so there's maybe not much value in it being a functional interface?

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 185:

> 183:                 public static \{invokerName} \{invokerFactoryName}(MemoryLayout... layouts) {
> 184:                     class Holder {
> 185:                         static final FunctionDescriptor BASE_DESC = \{descriptorString(2, descriptor)};

Why is the descriptor inside the holder class but the MH is not?

test/jtreg/generator/testPrintf/TestPrintf.java line 51:

> 49:  * @run testng/othervm --enable-native-access=ALL-UNNAMED TestPrintf
> 50:  */
> 51: public class TestPrintf {

Nice test!

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

PR Review: https://git.openjdk.org/jextract/pull/149#pullrequestreview-1759887838
PR Review Comment: https://git.openjdk.org/jextract/pull/149#discussion_r1412141314
PR Review Comment: https://git.openjdk.org/jextract/pull/149#discussion_r1412141795
PR Review Comment: https://git.openjdk.org/jextract/pull/149#discussion_r1412149206