RFR: making jextract work with latest panama-foreign repo targetting jdk 20 [v2]

[Maurizio Cimadamore]

On Fri, 12 Aug 2022 10:15:56 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

>> making jextract work with latest panama-foreign repo targetting jdk 20
>
> Athijegannathan Sundararajan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   version info changed in README.md

Good job.
I like how the tests (and samples) are simpler than they used to be, as a lot of address -> segment conversion is now gone.

README.md line 7:

> 5: ### Getting started
> 6: 
> 7: `jextract` depends on the [C libclang API](https://clang.llvm.org/doxygen/group__CINDEX.html). To build the jextract sources, the easiest option is to download LLVM binaries for your platform, which can be found [here](https://releases.llvm.org/download.html) (a version >= 9 is required). Both the `jextract` tool and the bindings it generates depend heavily on the [Foreign Function & Memory API](https://openjdk.java.net/jeps/424), so a suitable jdk build from panama-foreign repo is also required.

Suggestion:

`jextract` depends on the [C libclang API](https://clang.llvm.org/doxygen/group__CINDEX.html). To build the jextract sources, the easiest option is to download LLVM binaries for your platform, which can be found [here](https://releases.llvm.org/download.html) (a version >= 9 is required). Both the `jextract` tool and the bindings it generates depend heavily on the [Foreign Function & Memory API](https://openjdk.java.net/jeps/424), so a suitable build of the [panama/foreign repository](https://github.com/openjdk/panama-foreign) is also required.

samples/libffmpeg/LibffmpegMain.java line 123:

> 121:                 var pStream = streamsArray.getAtIndex(C_POINTER, i);
> 122:                 // AVStream stream;
> 123:                 var stream = MemorySegment.ofAddress(pStream.address(), AVStream.sizeof(), session);

I understand this is a straight port, but I don't think this is needed any longer. E.g. you can just use `pStream` and pass it to the subsequent getter, I think? (same in other snippets in this sample)

src/main/java/org/openjdk/jextract/impl/Utils.java line 117:

> 115:                 "VarHandle", "ByteOrder",
> 116:                 "FunctionDescriptor", "LibraryLookup",
> 117:                 "MemoryLayout", "MemorySession",

Is this correct? Where does MemorySesison comes from?

src/main/resources/org/openjdk/jextract/impl/resources/RuntimeHelper.java.template line 34:

> 32: 
> 33:     final static SegmentAllocator CONSTANT_ALLOCATOR =
> 34:             (size, align) -> MemorySegment.allocateNative(size, align, MemorySession.global());

These changes are probably not needed. E.g. implicit session seems good.

src/main/resources/org/openjdk/jextract/impl/resources/RuntimeHelper.java.template line 52:

> 50: 
> 51:     static final MemorySegment lookupGlobalVariable(String name, MemoryLayout layout) {
> 52:         return SYMBOL_LOOKUP.lookup(name).map(symbol -> MemorySegment.ofAddress(symbol.address(), layout.byteSize(), MemorySession.global())).orElse(null);

this is bogus: note that `symbol` already has a session. So the above should just use that.

src/main/resources/org/openjdk/jextract/impl/resources/RuntimeHelper.java.template line 52:

> 50: 
> 51:     static final MemorySegment lookupGlobalVariable(String name, MemoryLayout layout) {
> 52:         return SYMBOL_LOOKUP.lookup(name).map(symbol -> MemorySegment.ofAddress(symbol.address(), layout.byteSize(), MemorySession.global())).orElse(null);

Suggestion:

        return SYMBOL_LOOKUP.lookup(name).map(symbol -> MemorySegment.ofAddress(symbol.address(), layout.byteSize(), symbol.session())).orElse(null);

test/jtreg/generator/funcPointerInvokers/TestFuncPointerInvokers.java line 137:

> 135:         try (MemorySession session = MemorySession.openConfined()) {
> 136:             fp_addr$set(fp_addr.allocate((addr) -> MemorySegment.ofAddress(addr.address() + 1), session));
> 137:             assertEquals(fp_addr.ofAddress(fp_addr$get(), session).apply(MemorySegment.ofAddress(42)).address(), MemorySegment.ofAddress(43).address());

no need for calling `address` before assertEquals

test/jtreg/generator/funcPointerInvokers/TestFuncPointerInvokers.java line 137:

> 135:         try (MemorySession session = MemorySession.openConfined()) {
> 136:             fp_addr$set(fp_addr.allocate((addr) -> MemorySegment.ofAddress(addr.address() + 1), session));
> 137:             assertEquals(fp_addr.ofAddress(fp_addr$get(), session).apply(MemorySegment.ofAddress(42)).address(), MemorySegment.ofAddress(43).address());

Suggestion:

            assertEquals(fp_addr.ofAddress(fp_addr$get(), session).apply(MemorySegment.ofAddress(42)), MemorySegment.ofAddress(43));

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

PR: https://git.openjdk.org/jextract/pull/60