RFR: 7903933: Move sharable items from different generations to a common file [v4]

[Maurizio Cimadamore]

On Thu, 27 Mar 2025 05:55:08 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

>> 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.
>
> Nizar Benalla 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:
> 
>  - Merge branch 'master' into shareable-items
>  - add new option to create a sharable utility class
>    
>    cleanup: longer cmd option `framework` should use -- rather than -
>    
>    small other cleanups
>  - Merge branch 'master' into shareable-items
>  - Revert all changes - starting over from scratch
>  - replace hardcoded strings I missed
>  - move sharable items to a common class

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):

1. library arena
2. trace downcall support
3. alignment method
4. findOrThrow
5. upcallHandle
6. 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.

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

PR Comment: https://git.openjdk.org/jextract/pull/278#issuecomment-2757539685