7903933: Move sharable items from different generations to a common file #278

nizarbenalla · 2025-02-03T18:16:48Z

Please review this patch to move the C_* layouts and the static utility methods into separate classes: LayoutUtils.java and FFMUtils.java, respectively.

The names could later be personalized through a JSON configuration.
We can use static imports if the -t option is no used and the files are generated into the default package, in that case we use the classname to call the static methods or use the C_* constants.

Some tests had to be modified slightly, either by adding new static imports or replacing classnames.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (no review required)

Issue

CODETOOLS-7903933: Move sharable items from different generations to a common file (Enhancement - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jextract.git pull/278/head:pull/278
$ git checkout pull/278

Update a local copy of the PR:
$ git checkout pull/278
$ git pull https://git.openjdk.org/jextract.git pull/278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 278

View PR using the GUI difftool:
$ git pr show -t 278

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jextract/pull/278.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-03T18:17:31Z

👋 Welcome back nbenalla! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-03T18:18:35Z

@nizarbenalla This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

7903933: Move sharable items from different generations to a common file

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

ec585cc: 7903877: jextract exception handling in downcall wrappers

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2025-02-03T19:00:40Z

Webrevs

bridgekeeper · 2025-03-03T23:39:36Z

@nizarbenalla This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

cleanup: longer cmd option `framework` should use -- rather than - small other cleanups

nizarbenalla · 2025-03-27T05:51:09Z

I wasn't happy with the previous approach/implementation and started over from scratch, also did some small cleanup (the newly added option for framworks should use -- rather than -).

Shared items from multiple generation will only be moved to a class if the user specifies the --sharable-items option, with the class name of his choosing and the main header file extends that class. This will be helpful for project that need to run jextract on multiple headers, especially if those headers are large.

mcimadamore · 2025-03-27T10:33:26Z

Adding an option to move shared items in a separate class feels like taking a shortcut. Let's look what's inside this bag of shared items (it used to be much bigger, so good news there):

library arena
trace downcall support
alignment method
findOrThrow
upcallHandle
primitive layout constants

I believe (1) should probably not belong in a separate class from the headers. The way I see it is that it should belong where the library lookups belongs to. Even if you run multiple extractions it is less clear to me as to whether there should be a single lifetime for all the different libraries or not (and, eventually, we want aspects such as this to be customizable via subclassing).

(2) seems a very useful debug option, and I think we should think about making it more official -- for instance with a linker option.

(3) is a general purpose method that would make sense to feature in the main MemoryLayout API.

(4) seems to be superseded by a similar method that was added in Java 23:
https://docs.oracle.com/en/java/javase/23/docs//api/java.base/java/lang/foreign/SymbolLookup.html#findOrThrow(java.lang.String)

(5) seems like it could be inlined in the functional interface classes (it's just an helper to allow clients to create an upcall stub w/o the need to do a MethodHandle lookup -- which needs a try/catch).

Then there's (6). The first reaction I got was: well, whether primitive layouts should be emitted or not seems like another filtering decision (e.g. let's add some options to filter these out). Except this would not work -- it's not just about filtering -- it's also about telling every other file where to find the layouts for such primitive types. If they are defined somewhere else, then jextract need to know where to find them (e.g. if they are referenced by some other layout jextract is building). Which seems a similar problem as the one this PR is trying to solve anyway.

Stepping back -- I think if we look back, jetxract used to generate a RuntimeHelper class, and then everything else. While we moved away from that generation scheme (now we just emit header classes), the shared functionalities are still there. Perhaps a move in a good direction would be to:

put all the shared functionalities in the root of the header hierarchy -- and don't put anything else in there
this means that there will always be at least two header classes generated foo_f and foo_h$0, where foo_h extends foo_h$0 and all the shared symbols are in foo_h$0.
add an option to override the name of that root header class

This is similar, in spirit, to what you have here, but with the advantage that there's only one generation scheme, not two -- e.g. the superclass with the shared symbol is always there -- only sometimes it can have a different name (because the user said so). Whether we want that default superclass name to be foo_h$0, or maybe something more explicit like foo_h_shared, I'm open to suggestions.

But I do think that we should try and make that shared class as small as possible -- most of the functionality there seems like it could belong in the main FFM API, and it would provide value even for clients not running on top of jextract.

mcimadamore · 2025-03-27T10:36:29Z

Then there's (6). The first reaction I got was: well, whether primitive layouts should be emitted or not seems like another filtering decision (e.g. let's add some options to filter these out). Except this would not work -- it's not just about filtering -- it's also about telling every other file where to find the layouts for such primitive types. If they are defined somewhere else, then jextract need to know where to find them (e.g. if they are referenced by some other layout jextract is building). Which seems a similar problem as the one this PR is trying to solve anyway.

Note/history: while tempting, we can't really put (6) inside the main FFM API. We have thought about this for a long time -- the issue is that the types of the primitive layouts is not guaranteed to be stable. E.g. C_LONG might be either a ValueLayout.OfLong or a ValueLayout.OfInt, depending on the platforms. Other layouts, such as C_LONG_DOUBLE might only be available on some platforms and not others. This is why, long ago, we decided that the main FFM API should not concern with providing layout constants for C types -- as the set of such constant is not stable. Instead, such primitive C layouts can be "discovered" using the Linker::canonicalLayouts API.

mcimadamore · 2025-03-27T10:45:05Z

Then there's (6). The first reaction I got was: well, whether primitive layouts should be emitted or not seems like another filtering decision (e.g. let's add some options to filter these out). Except this would not work -- it's not just about filtering -- it's also about telling every other file where to find the layouts for such primitive types. If they are defined somewhere else, then jextract need to know where to find them (e.g. if they are referenced by some other layout jextract is building). Which seems a similar problem as the one this PR is trying to solve anyway.

Also on this topic: for now we're mostly concerned about different extractions not repeating the code for primitive layouts and helper functions. This feels more like a "tip of the iceberg" kind of situation. For instance, you might have two libraries A and B, which both include the header of some third library C. Maybe you want to extract C separately, and then extract A and B so that they somehow magically point at the extracted bindings for C. Now sharing would be not just about primitive types, but about functions, structs and much more.

At the same time, going down this path can be very complex: A and B might pull in slightly different versions of C, or use some #define macro directives which would alter the shape of the generated bindings in C. In which case reusing the same bindings for C would be more difficult.

So, hidden somewhere in here there's a theme of: how do we move jextract to go from a per-extraction set of bindings to a multi-extraction friendly model. And going down this path will likely, I think, result in opening a big and complicated can of worms. I'm not saying we'll never get there -- but there's a limit with what we can express with simple command line options.

JornVernee · 2025-03-27T14:49:36Z

this means that there will always be at least two header classes generated foo_f and foo_h$0, where foo_h extends foo_h$0 and all the shared symbols are in foo_h$0.

Should we just give the base header a common name then, so that if you generate multiple times, you get sharing automatically? Maybe it could be named something like Builtins.

mcimadamore · 2025-03-27T15:04:00Z

this means that there will always be at least two header classes generated foo_f and foo_h$0, where foo_h extends foo_h$0 and all the shared symbols are in foo_h$0.

Should we just give the base header a common name then, so that if you generate multiple times, you get sharing automatically? Maybe it could be named something like Builtins.

Yes, see

This is similar, in spirit, to what you have here, but with the advantage that there's only one generation scheme, not two -- e.g. the superclass with the shared symbol is always there -- only sometimes it can have a different name (because the user said so). Whether we want that default superclass name to be foo_h$0, or maybe something more explicit like foo_h_shared, I'm open to suggestions.

nizarbenalla · 2025-03-27T16:37:40Z

(4) findOrThrow
(4) seems to be superseded by a similar method that was added in Java 23:
https://docs.oracle.com/en/java/javase/23/docs//api/java.base/java/lang/foreign/SymbolLookup.html#findOrThrow(java.lang.String)

I meant to remove this when targeting jdk 23, it seems I left it by mistake. I will remove it to reduce the number of shared items we're dealing with.

- Add option to override the name of the shared symbol class

nizarbenalla · 2025-04-16T19:35:05Z

I've moved the shared symbols to a different class foo_h$shared that all headers extend from.

The new command line option was renamed to --shared-symbolsto allow users to specify the name of the class if they want to.

mcimadamore · 2025-04-28T15:53:47Z

The option for changing header class name is called --header-class-name <name>. So I'd suggest something like --symbols-class-name.

mcimadamore · 2025-04-28T15:58:01Z

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java

    private static HeaderFileBuilder createFirstHeader(SourceFileBuilder sfb, List<Options.Library> libs, boolean useSystemLoadLibrary) {
-        HeaderFileBuilder first = new HeaderFileBuilder(sfb, String.format("%1$s#{SUFFIX}",sfb.className()), null, sfb.className());
+        HeaderFileBuilder first = new HeaderFileBuilder(sfb, String.format("%1$s#{SUFFIX}",sfb.className()), SHARED, sfb.className());
        first.appendBlankLine();
        first.classBegin();
        first.emitDefaultConstructor();
        first.emitRuntimeHelperMethods();


Shouldn't the runtime helper methods end up in the shared class?

Fixed in 5c8802a, I thought only the shared symbols belonged in the separate class.

I did not move the library arena to the separate class and instead kept it close to the symbol lookup.

mcimadamore · 2025-04-28T15:59:58Z

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java

+    private void createSharedClass() {
+        SourceFileBuilder sfb = SourceFileBuilder.newSourceFile(packageName(), SHARED);
+        HeaderFileBuilder sharedBuilder = new HeaderFileBuilder(sfb, SHARED, null, SHARED);
+        sharedBuilder.appendBlankLine();


There seems to be a lot of overlapping between the code here and the one in nextHeader -- especially now that the first header is not really meant to have a lot of special things (the only exception being the symbol lookup logic).

Fixed, I consolidated the shared code in some methods. I ran into issues with the java files having the wrong name so I had to change the logic a little.

mcimadamore

Very nice cleanup! Looks good

nizarbenalla · 2025-05-05T12:44:50Z

I pushed a small test-only update, I was seeing some failures in the CI (that I didn't see locally). This fixes it,

mcimadamore

Good catch!

nizarbenalla · 2025-05-06T11:17:14Z

Thanks for all the rounds of reviews.

/integrate

openjdk · 2025-05-06T11:18:49Z

@nizarbenalla
Your change (at version e3b9ae2) is now ready to be sponsored by a Committer.

JornVernee · 2025-05-06T17:39:00Z

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java

+    void emitLibaryArena(){appendIndentedLines("""
+
+            static final Arena LIBRARY_ARENA = Arena.ofAuto();""");
+    }


Suggested change

void emitLibaryArena(){appendIndentedLines("""

static final Arena LIBRARY_ARENA = Arena.ofAuto();""");

}

void emitLibaryArena(){

appendIndentedLines("""

static final Arena LIBRARY_ARENA = Arena.ofAuto();""");

}

JornVernee · 2025-05-06T17:41:54Z

src/main/java/org/openjdk/jextract/impl/IncludeHelper.java

+    public String sharedSymbolsFile;
+
+    public String getSharedSymbolsFile() {
+        return sharedSymbolsFile;
+    }


Uhm, I think this should just be in Options, not in IncludeHelper, since it's not related to the --include options.

JornVernee · 2025-05-06T17:44:35Z

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java

@@ -43,48 +43,76 @@ class ToplevelBuilder implements OutputFactory.Builder {
    private static final int DECLS_PER_HEADER_CLASS = Integer.getInteger("jextract.decls.per.header", 1000);
    public static final String PREV_SUFFIX = "#{PREV_SUFFIX}";
    private static final String SUFFIX = "#{SUFFIX}";
+    private static String SHARED;


I think this was supposed to be final?

Suggested change

private static String SHARED;

private final String shared;

JornVernee · 2025-05-06T17:44:57Z

doc/GUIDE.md

+| `--output <path>`                                                | specify where to place generated files                                                                                                                                                                                                                                                |
+| `--dump-includes <String>`                                       | dump included symbols into specified file (see below)                                                                                                                                                                                                                                 |
+| `--include-[function,constant,struct,union,typedef,var]<String>` | Include a symbol of the given name and kind in the generated bindings. When one of these options is specified, any symbol that is not matched by any specified filters is omitted from the generated bindings.                                                                        |
+| `--symbols-class-name <name>`                                          | override the name of the root header class                                                                                                                                                                                                                                            |


JornVernee · 2025-05-06T17:50:19Z

src/main/resources/org/openjdk/jextract/impl/resources/Messages.properties

@@ -84,6 +85,7 @@ Option                             Description
 \                                   option is not specified, then current directory is used.    \n\
 -t, --target-package <package>     target package name for the generated classes. If this option\n\
 \                                   is not specified, then unnamed package is used.             \n\
+--symbols-class-name <name>            override the name of the root header class                   \n\


Suggested change

--symbols-class-name <name> override the name of the root header class \n\

--symbols-class-name <name> override the name of the root header class \n\

JornVernee · 2025-05-06T17:53:33Z

src/main/java/org/openjdk/jextract/impl/FunctionalInterfaceBuilder.java

@@ -42,7 +42,7 @@ final class FunctionalInterfaceBuilder extends ClassSourceBuilder {

    private FunctionalInterfaceBuilder(SourceFileBuilder builder, String className, ClassSourceBuilder enclosing,
                                       String runtimeHelperName, Type.Function funcType, boolean isNested) {
-        super(builder, isNested ? "public static" : "public", Kind.CLASS, className, null, enclosing, runtimeHelperName);
+        super(builder, isNested ? "public final static" : "public final", Kind.CLASS, className, null, enclosing, runtimeHelperName);


Changes in this file seem unrelated? Leftover from other work?

JornVernee · 2025-05-06T17:54:07Z

test/testng/org/openjdk/jextract/test/toolprovider/TestClassGeneration.java

@@ -192,7 +192,7 @@ public void testStructMember(String structName, MemoryLayout memberLayout, Class
    @Test(dataProvider = "functionalInterfaces")
    public void testFunctionalInterface(String name, MethodType type) {
        Class<?> fpClass = loader.loadClass("com.acme." + name);
-        checkDefaultConstructor(fpClass);
+        checkPrivateConstructor(fpClass);


Same here, these changes seem unrelated.

move sharable items to a common class

583bcf6

nizarbenalla marked this pull request as ready for review February 3, 2025 18:55

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 3, 2025

replace hardcoded strings I missed

fa41fe8

nizarbenalla added 4 commits March 27, 2025 03:04

Revert all changes - starting over from scratch

d427310

Merge branch 'master' into shareable-items

748bcc9

add new option to create a sharable utility class

736ccfb

cleanup: longer cmd option `framework` should use -- rather than - small other cleanups

Merge branch 'master' into shareable-items

69d4647

nizarbenalla changed the title ~~7903933: Move sharable items from different generations to a common file~~ 7903933: Generate callbacks using interface + upcall record Apr 9, 2025

nizarbenalla changed the title ~~7903933: Generate callbacks using interface + upcall record~~ 7903933: Move sharable items from different generations to a common file Apr 9, 2025

nizarbenalla added 2 commits April 14, 2025 13:55

Merge branch 'master' into shareable-items

09c7fca

- Move shared symbols to a different class

01c8205

- Add option to override the name of the shared symbol class

make utility classes final with a private constructor

532ee1a

mcimadamore reviewed Apr 28, 2025

View reviewed changes

feedback: rename option and consolidate code

5c8802a

mcimadamore approved these changes May 2, 2025

View reviewed changes

update test

e3b9ae2

mcimadamore approved these changes May 6, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label May 6, 2025

JornVernee reviewed May 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7903933: Move sharable items from different generations to a common file #278

7903933: Move sharable items from different generations to a common file #278

nizarbenalla commented Feb 3, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 3, 2025

openjdk bot commented Feb 3, 2025 •

edited

Loading

mlbridge bot commented Feb 3, 2025 •

edited

Loading

bridgekeeper bot commented Mar 3, 2025

nizarbenalla commented Mar 27, 2025

mcimadamore commented Mar 27, 2025

mcimadamore commented Mar 27, 2025 •

edited

Loading

mcimadamore commented Mar 27, 2025

JornVernee commented Mar 27, 2025

mcimadamore commented Mar 27, 2025

nizarbenalla commented Mar 27, 2025

nizarbenalla commented Apr 16, 2025 •

edited

Loading

mcimadamore commented Apr 28, 2025

mcimadamore Apr 28, 2025

nizarbenalla May 2, 2025

mcimadamore Apr 28, 2025

nizarbenalla May 2, 2025

mcimadamore left a comment

nizarbenalla commented May 5, 2025

mcimadamore left a comment

nizarbenalla commented May 6, 2025 •

edited

Loading

openjdk bot commented May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

JornVernee May 6, 2025

	\| `--symbols-class-name <name>` \| override the name of the root header class \|
	\| `--symbols-class-name <name>` \| override the name of the root header class \|

	--symbols-class-name <name> override the name of the root header class \n\
	--symbols-class-name <name> override the name of the root header class \n\

7903933: Move sharable items from different generations to a common file #278

Are you sure you want to change the base?

7903933: Move sharable items from different generations to a common file #278

Conversation

nizarbenalla commented Feb 3, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 3, 2025

openjdk bot commented Feb 3, 2025 • edited Loading

mlbridge bot commented Feb 3, 2025 • edited Loading

Webrevs

bridgekeeper bot commented Mar 3, 2025

nizarbenalla commented Mar 27, 2025

mcimadamore commented Mar 27, 2025

mcimadamore commented Mar 27, 2025 • edited Loading

mcimadamore commented Mar 27, 2025

JornVernee commented Mar 27, 2025

mcimadamore commented Mar 27, 2025

nizarbenalla commented Mar 27, 2025

nizarbenalla commented Apr 16, 2025 • edited Loading

mcimadamore commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

nizarbenalla commented May 5, 2025

mcimadamore left a comment

Choose a reason for hiding this comment

nizarbenalla commented May 6, 2025 • edited Loading

openjdk bot commented May 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nizarbenalla commented Feb 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 3, 2025 •

edited

Loading

mlbridge bot commented Feb 3, 2025 •

edited

Loading

mcimadamore commented Mar 27, 2025 •

edited

Loading

nizarbenalla commented Apr 16, 2025 •

edited

Loading

nizarbenalla commented May 6, 2025 •

edited

Loading