[foreign-jextract] RFR: 8263018: Improve API for lifecycle of native resources [v2]

Wed Mar 24 13:24:57 UTC 2021

On Wed, 24 Mar 2021 12:32:07 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This contains the jextract changes required to support the new changes to the foreign memory access API.
>> 
>> The main change is that now NativeScope is co-generated by jextract (although, moving forward, I'm not sure how much value it adds, but that's for another day).
>> 
>> I had to fix the extracted libclang in few places (probably better to re-generate this code once this is pushed).
>> 
>> Tests needed changes to fix imports so that they pick up the generated NativeScope class.
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 98 commits:
> 
>  - Merge branch 'foreign-jextract' into jextract+resourceScope
>  - Merge branch 'foreign-memaccess+abi' into jextract+resourceScope
>  - Merge branch 'foreign-jextract' into jextract+resourceScope
>  - Add overloads accepting ResourceScope parameter
>  - Merge branch 'resourceScope' into jextract+resourceScope
>  - Add ResourceScope overloads in CLinker
>    Rewrite StdLib test not to use NativeScope
>    Tweak names of allocator factories
>    Fix javadoc
>  - All tests pass
>    * Fix upcall overload taking NativeScope
>    * Generate NativeScope in output package
>    * Add overload for functions returning a struct so than a NativeScope can be passed
>    * Removed some now unused features from RuntimeHelper (ti make scope non-transferrable)
>    * Fixup vararg invoker to deal with allocator parameters
>    * Fixup tests to (a) depend on the generated NativeScope and (b) not to use TWR with segments directly
>    Notes:
>    * the internal libclang probably needs to be re-generated
>  - Merge branch 'resourceScope' into jextract+resourceScope
>  - Fix javadoc of SegmentAllocator
>  - Address rest of review comments from Paul
>  - ... and 88 more: https://git.openjdk.java.net/panama-foreign/compare/4e696684...6316ddca

I think there are some issues, particularly wrt to using managed memory segments that are projected down to addresses. I don't think managed segments should be used in those cases. But, we can get the branch building first and address later maybe.

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/TranslationUnit.java line 69:

> 67:     public final void save(Path path) throws TranslationUnitSaveException {
> 68:         MemorySegment pathStr = CLinker.toCString(path.toAbsolutePath().toString());
> 69:         SaveError res = SaveError.valueOf(Index_h.clang_saveTranslationUnit(tu, pathStr, 0));

This function takes the address of the pathStr and passes it to a native function, so I don't believe using a GC managed segment here is safe.

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/Type.java line 115:

> 113:     private long getOffsetOf0(String fieldName) {
> 114:         MemorySegment cfname = CLinker.toCString(fieldName);
> 115:         return Index_h.clang_Type_getOffsetOf(type, cfname);

Same here, this function takes the address of `cfname` so can't use a managed segment

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/libclang/RuntimeHelper.java line 93:

> 91:                     return variadic ?
> 92:                         VarargsInvoker.make(addr, mt, fdesc) :
> 93:                         LINKER.downcallHandle(addr, DEFAULT_ALLOCATOR, mt, fdesc);

Using a managed segment by default here also seems like asking for trouble, since we don't know how the returned struct is going to be used (e.g. might be projected into an address).

Would seem more robust to have a throwing allocator used here for functions that don't have MS as a return type, and then determine on a case-by-case basis which scope to use for MS returns. (sorry)

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/libclang/RuntimeHelper.java line 97:

> 95:     }
> 96: 
> 97:     static final MethodHandle downcallHandle(LibraryLookup[] LIBRARIES, String name, String desc, FunctionDescriptor fdesc, boolean variadic, NativeScope scope) {

This seems unused. Should this have gone into the template file instead maybe?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/FunctionalInterfaceBuilder.java line 113:

> 111:                 delim = ", ";
> 112:             }
> 113:             append(") -> {\n");

Looks like this is also switching from anon classes to lambdas?

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

> 97:                 emitFunctionWrapperNoAllocatorOverload(javaName, functionInfo);
> 98:                 emitFunctionWrapperScopedOverload(javaName, functionInfo);
> 99:             }

With the emitFunctionWrapper call above this, don't we get a publicly accessible function wrapper that will always throw a WrongMethodTypeException because it's missing an allocator argument? Maybe it should be conditionally made private? (since the other 2 functions reference it, it's still needed).

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

> 137:             declType = declType.insertParameterTypes(0, SegmentAllocator.class);
> 138:             paramNames = new ArrayList<>(paramNames);
> 139:             paramNames.add("allocator");

Shouldn't this be added as the first element rather than the last?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/OutputFactory.java line 159:

> 157:                         .replace("${C_LANG}", C_LANG_CONSTANTS_HOLDER);
> 158:     }
> 159: 

This could be partially merged with `getRuntimeHelperSource`

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypedefBuilder.java line 78:

> 76:         append("}\n");
> 77:         decrAlign();
> 78:     }

I don't get why this is removed. Was it not being used?

test/jdk/tools/jextract/test8244959/Test8244959.java line 58:

> 56:             (byte) 1, 'b', -1.25f, 5.5d, -200L, Long.MAX_VALUE, (byte) -2, (short) 2, 3, (short) -4, 5L, 'a');
> 57:         String str = toJavaString(s);
> 58:         assertEquals(str, "1 b -1.25 5.50 -200 " + Long.MAX_VALUE + " -2 2 3 -4 5 a");

I think using managed segments here could be problematic as well.

test/jdk/tools/jextract/test8246341/LibTest8246341Test.java line 64:

> 62:             assertEquals(toJavaStringRestricted(MemoryAccess.getAddressAtIndex(addr, 3)), "c++");
> 63:         });
> 64:         func(callback);

Same here; managed segment but this is projecting to a MemoryAdress AFAIK.

test/jdk/tools/jextract/testFunctionPointer/LibFuncPtrTest.java line 47:

> 45:     public void test() {
> 46:         var handle = func$f.allocate(x -> x*x);
> 47:         assertEquals(func(handle, 35), 35 * 35 + 35);

Again here, managed segment projected to address

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

Marked as reviewed by jvernee (Committer).

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