[foreign-jextract] RFR: 8274155 Foreign API refresh - jextract update

[Maurizio Cimadamore]

On Thu, 23 Sep 2021 02:59:21 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

>> These are the jextract changes for the recent refresh of the Foreign Memory Access/Foreign Linker API. The biggest change in this API refresh comes from the removal of layout attributes. Jextract used to store several information in the form of layouts attributes. After some analysis, it turned out that in all cases, the use of layout attributes was not necessary, and it was really driven by the fact that jextract was, in certain places, using `MemoryLayout` as its main internal representation, instead of using the more flexible `Declaration`/`Type` APIs. Unfortunately, updating `RecordLayoutComputer` to work on `Declaration` rather than `MemoryLayout` caused changes in several part of the jextract parsing frontend.
>> 
>> Other minor changes were required, to adjust jextract to the new API changes - but most of these are small. I've dropped several now redundant overloads (so the size of the generated bindings should be smaller). There's also an additional piece of code which is used to emit typedef for common C primitive types such as `C_INT` and the likes (since the layouts for these are no longer in `CLinker`).
>> 
>> The test changes are mostly trivial updates to use the new API.
>
> src/jdk.incubator.jextract/share/classes/jdk/internal/clang/libclang/CLayouts.java line 5:
> 
>> 3: import jdk.incubator.foreign.ValueLayout;
>> 4: 
>> 5: public class CLayouts {
> 
> could this be an interface?

yes - this can be an interface.

> src/jdk.incubator.jextract/share/classes/jdk/internal/clang/libclang/RuntimeHelper.java line 55:
> 
>> 53: import static jdk.internal.clang.libclang.CLayouts.C_LONG_LONG;
>> 54: import static jdk.internal.clang.libclang.CLayouts.C_POINTER;
>> 55: 
> 
> I assume this is transition code. RuntimeHelper is generated. It cannot use handwritten CLayouts interface (unless we plan to generate something similar from jextract every time)

Yep, this is transitional code - jextract will generate these layouts now.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ConstantBuilder.java line 332:
> 
>> 330:         append("MemorySegment ");
>> 331:         append(fieldName);
>> 332:         append(" = ResourceScope.newSharedScope().allocateUtf8String(\"");
> 
> Do we need shared scope here? or confined scope will do?

This is a string constant - so we don't know which thread will use it. It cannot be confined. Thinking more, perhaps we should even use the global scope - because we don't really want these to ever be cleaned up.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 371:
> 
>> 369: 
>> 370:         if (type instanceof Type.Declared) {
>> 371:             // anon type - let's generate something
> 
> name empty check removed. Does this comment make sense still?

I will need to double check this one.

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/RecordLayoutComputer.java line 191:
> 
>> 189: //        }
>> 190: //        return c.spelling();
>> 191: //    }
> 
> debug code leftover?

good catch

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ToplevelBuilder.java line 189:
> 
>> 187:             emitPrimitiveTypedef(Type.primitive(Type.Primitive.Kind.Double), "C_DOUBLE");
>> 188:             emitPointerTypedef("C_POINTER");
>> 189:         }
> 
> Will this clash with user's typedef?
> 
> typedef int C_INT; 
> 
> in user's header?

possibly, perhaps we can introduce some disambiguation later?

> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypeImpl.java line 45:
> 
>> 43: 
>> 44:     public static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
>> 45: 
> 
> is this used?

yes, ```Long("long", TypeImpl.IS_WINDOWS ? ValueLayout.JAVA_INT : ValueLayout.JAVA_LONG),```

> test/jdk/tools/jextract/JextractToolRunner.java line 68:
> 
>> 66:     public static final ValueLayout.OfDouble C_DOUBLE = ValueLayout.JAVA_DOUBLE;
>> 67:     public static final ValueLayout.OfAddress C_POINTER = ValueLayout.ADDRESS;
>> 68: 
> 
> where are these constant used?

in tests which extends this class, I believe

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

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