[foreign-jextract] RFR: 8264787: update jextract for foreign-memory-abi API changes and --enable-native-access option

[Maurizio Cimadamore]

On Tue, 6 Apr 2021 17:56:56 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

> 8264787: update jextract for foreign-memory-abi API changes and --enable-native-access option

Looks good, thanks for taking care of this. I've added some comments, most of which can be handled separately. I did not reviewed the changes in Index_h, which is auto-generated, on the basis that we will autogenerate that again anyway.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/HeaderFileBuilder.java line 97:

> 95:             if (functionInfo.methodType().returnType().equals(MemorySegment.class)) {
> 96:                 // emit scoped overload
> 97:                 // emitFunctionWrapperNoAllocatorOverload(javaName, functionInfo);

Any reason as to why this is commented as opposed to removed (along with the corresponding, now unused, method) ?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/StructBuilder.java line 286:

> 284:     }
> 285: 
> 286:     private void emitOfAddress() {

I think this needs to go to, as we're kind of inferring that users want global scope. Ok if this is handled as followup patch.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/resources/RuntimeHelper.java.template line 37:

> 35:     }
> 36: 
> 37:     private final static SegmentAllocator DEFAULT_ALLOCATOR = (x, y) -> MemorySegment.allocateNative(x, y, ResourceScope.newImplicitScope());

If this is only used in the varargs case, then I think having a THROWING_ALLOCATOR which always throws is also an option as I don't think we really want this to ever be used.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/resources/RuntimeHelper.java.template line 81:

> 79:     }
> 80: 
> 81:     static final <Z> MemorySegment upcallStub(Class<Z> fi, Z z, FunctionDescriptor fdesc, String mtypeDesc, NativeScope scope) {

This reminds me, perhaps at some point we need to do another pass and get rid of NativeScope

test/jdk/java/jextract/SmokeTest.java line 27:

> 25:  * @test
> 26:  * @build JextractApiTestBase
> 27:  * @run testng/othervm --enable-native-access=jdk.incubator.jextract SmokeTest

This makes me wonder if jextract should be white-listed. After all it's JDK-provided, so all usages of restricted API should be correct, and if there's a crash in the jextract tool, well, that's a bug in the tool, no?

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

Marked as reviewed by mcimadamore (Committer).

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