RFR: 7903606: Move layout and function descriptor generation closer to code builders

[Maurizio Cimadamore]

On Mon, 11 Dec 2023 14:31:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This PR moves calls to Type/Declaration.layoutFor/descriptorFor as close as possible to the leaf code builders. The reason for this change is to prepare the jextract code to generate layouts and descrptor strings directly from jextract types, so that we can retain as much information from the original clang cursor/types as possible when generating layouts.
> 
> In order to get there, I had to add a lot of type predicates in the Utils class (perhaps some of these methods can be moved later directly in TypeImpl).
> 
> And, perhaps the biggest change, is that previously we used layout presence/absence in `UnsupportedFilter` to make decision as to whether skip a certain declaration. For this reason, `UnsupportedFilter` (esp. its type visitor) has been enhanced to detect cases where layout generation would fail, but in a way that does not require us to generate layouts just to answer a question of "is this type supported?".

src/main/java/org/openjdk/jextract/impl/FunctionalInterfaceBuilder.java line 40:

> 38: final class FunctionalInterfaceBuilder extends ClassSourceBuilder {
> 39: 
> 40:     private final Type.Function funcType;

Lot of duplication in this class - this patch removes some of that

src/main/java/org/openjdk/jextract/impl/OutputFactory.java line 219:

> 217:     interface Builder {
> 218: 
> 219:         default void addVar(Declaration.Variable varTree, Optional<String> fiName) {

The main thing here is that we no longer compute layouts before the fact, and pass it to the builder - the builder will just compute this info as required by accessing the corresponding decl

src/main/java/org/openjdk/jextract/impl/UnsupportedFilter.java line 62:

> 60:     }
> 61: 
> 62:     static Type firstUnsupportedType(Type type, boolean allowVoid) {

Note - the visitor now returns the unsupported type directly, instead of returning its string representation. This seems better.

src/main/java/org/openjdk/jextract/impl/UnsupportedFilter.java line 100:

> 98:     @Override
> 99:     public Void visitVariable(Variable varTree, Declaration parent) {
> 100:         Type type = varTree.type();

A bit of a snag with the IR - in a lot of situations we have to recursively visit the declaration corresponding to a declared type (if any is found), as that could be some anonymous struct/union. It is possible this might lead to the same declaration being visited twice.

src/main/java/org/openjdk/jextract/impl/UnsupportedFilter.java line 148:

> 146:     public Void visitTypedef(Typedef typedefTree, Declaration declaration) {
> 147:         // propagate
> 148:         if (typedefTree.type() instanceof Declared declared) {

propagation logic should happen early, otherwise might be skipped.

src/main/java/org/openjdk/jextract/impl/UnsupportedFilter.java line 227:

> 225:         }
> 226: 
> 227:         private boolean isValidStructOrUnion(Scoped scoped) {

This is a method which checks whether a struct or union is valid. Basically the conditions are:
* its size should be available (e.g. not an opaque struct)
* if it's an anonymous struct, it should have a known first offset (otherwise, the anon struct should be just skipped)

test/jtreg/generator/test8257892/LibUnsupportedTest.java line 94:

> 92: 
> 93:     @Test
> 94:     public void testProblematicMethods() {

In this case we were checking that some methods were NOT generated, even though we could perfectly compute their layouts.

test/testng/org/openjdk/jextract/test/toolprovider/Test7903257/TestDocComments.java line 72:

> 70:             "typedef unsigned long long size_t;",
> 71:             "typedef int INT_32;",
> 72:             "typedef int* INT_PTR;"));

opaque structs are now skipped, so there's no interface for them

test/testng/org/openjdk/jextract/test/toolprovider/Test8251943.java line 45:

> 43:         try(TestUtils.Loader loader = TestUtils.classLoader(outputPath)) {
> 44:             Class<?> headerClass = loader.loadClass("test8251943_h");
> 45:             assertNotNull(findMethod(headerClass, "tzname$SEGMENT"));

Again, here we were not generating a segment accessor for something whose layout could be computed.

test/testng/org/openjdk/jextract/test/toolprovider/TestClassGeneration.java line 189:

> 187:         try (Arena arena = Arena.ofConfined()) {
> 188:             MemorySegment struct = arena.allocate(structLayout);
> 189:             Method vh_getter = checkMethod(structCls, memberName + "$VH", VarHandle.class);

This removal comes from the fact that StructBuilder no longer relies on var handle generation in struct field getter/setters (we just generate the correct layout inline).

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

PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422549234
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422550774
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422552073
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422553555
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422554557
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422556371
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422557426
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422557794
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422558354
PR Review Comment: https://git.openjdk.org/jextract/pull/156#discussion_r1422559271