Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:39:08 GMT, Mandy Chung wrote: >> `BootLoader` is what you want in this case - it defines the static methods >> to access resources, packages etc defined to the boot loader (aka null >> `ClassLoader`). >> >> To find a symbol defined in the native libraries loaded by the boot loader, >> you can call `BootLoader.getNativeLibraries().find(name)`. > > When a caller-sensitive method is invoked from a thread attached via JNI, the > caller class returned from `Reflection::getCallerClass` is `null`.I would > recommend the default to be a class in the unnamed module defined to the > system class loader; hence it defaults to the system class loader. This is > consistent with JNI `FindClass` when invoked with no caller frame. > > It's an open issue [1] to revisit the default for `System::load` and > `System::loadLibrary` when invoked with null caller class. However, the > existing behavior may likely be unchanged because of the compatibility risk. > > [1] https://bugs.openjdk.java.net/browse/JDK-8281001 Thanks for the comments. I've pushed a new change which fixes a couple of thing: * first, for `SymbolLookup.loaderLookup`, the system class loader is used when no caller class exists (e.g. when method is called from JNI). If caller class exist but its loader is null (boot loader), we just call ClassLoader::findNative with a `null` loader which will do the right thing (thanks @mlchung for the tips!) * second, the restricted method check in `Reflection::ensureNativeAccess` has been improved to also work in case where caller class is `null`. In such cases, a dummy unnamed module module is used for the check. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:22:41 GMT, Mandy Chung wrote: >> Looking deeper, `System::loadLibrary` seems to search the boot loader >> libraries when invoked with a `null` caller class, so replicating that >> behavior should be safe. > > `BootLoader` is what you want in this case - it defines the static methods to > access resources, packages etc defined to the boot loader (aka null > `ClassLoader`). > > To find a symbol defined in the native libraries loaded by the boot loader, > you can call `BootLoader.getNativeLibraries().find(name)`. When a caller-sensitive method is invoked from a thread attached via JNI, the caller class returned from `Reflection::getCallerClass` is `null`.I would recommend the default to be a class in the unnamed module defined to the system class loader; hence it defaults to the system class loader. This is consistent with JNI `FindClass` when invoked with no caller frame. It's an open issue [1] to revisit the default for `System::load` and `System::loadLibrary` when invoked with null caller class. However, the existing behavior may likely be unchanged because of the compatibility risk. [1] https://bugs.openjdk.java.net/browse/JDK-8281001 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:44:08 GMT, Maurizio Cimadamore wrote: >> Another option would be to treat calls to `ensureNativeAccess` with `null` >> caller class as coming from unnamed module. > > Looking deeper, `System::loadLibrary` seems to search the boot loader > libraries when invoked with a `null` caller class, so replicating that > behavior should be safe. `BootLoader` is what you want in this case - it defines the static methods to access resources, packages etc defined to the boot loader (aka null `ClassLoader`). To find a symbol defined in the native libraries loaded by the boot loader, you can call `BootLoader.getNativeLibraries().find(name)`. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:29:53 GMT, Maurizio Cimadamore wrote: >> Good points. Regarding `ClassLoader` being null, I think we can still return >> something using the `BootLoader`'s `NativeLibraries` object - that would >> allow this method to be called internally. @mlchung Can you please confirm? >> >> As for the caller sensitive having no real caller (e.g. because method is >> called directly from native code), I think we should add a check in >> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such >> a check does not compromise upcall stub created via the Linker interface >> (e.g. a Java upcall might require to perform restricted operations - but >> those operation always happen inside the upcall MH, which is always >> associated with some class). > > Another option would be to treat calls to `ensureNativeAccess` with `null` > caller class as coming from unnamed module. Looking deeper, `System::loadLibrary` seems to search the boot loader libraries when invoked with a `null` caller class, so replicating that behavior should be safe. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:20:21 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>>
>>> 151: static SymbolLookup loaderLookup() {
>>> 152: Class caller = Reflection.getCallerClass();
>>> 153: ClassLoader loader =
>>> Objects.requireNonNull(caller.getClassLoader());
>>
>> Shouldn’t this be changed to throw `IllegalCallerException` instead of
>> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
>> `null`[^1] or when `caller` itself is `null`[^2]?
>>
>> [^1]: This happens when `caller` is on the bootstrap classpath.
>> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
>> code and no **Java** code is on the call stack.
>
> Good points. Regarding `ClassLoader` being null, I think we can still return
> something using the `BootLoader`'s `NativeLibraries` object - that would
> allow this method to be called internally. @mlchung Can you please confirm?
>
> As for the caller sensitive having no real caller (e.g. because method is
> called directly from native code), I think we should add a check in
> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
> check does not compromise upcall stub created via the Linker interface (e.g.
> a Java upcall might require to perform restricted operations - but those
> operation always happen inside the upcall MH, which is always associated with
> some class).
Another option would be to treat calls to `ensureNativeAccess` with `null`
caller class as coming from unnamed module.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 16:47:28 GMT, ExE Boss wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 57 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Tweak support for Linker lookup
>>Improve javadoc of SymbolLookup
>> - Tweak Addressable javadoc
>> - Downcall and upcall tests use wrong layout which is missing some trailing
>> padding
>> - Simplify libraryLookup impl
>> - Address CSR comments
>> - Merge branch 'master' into foreign-preview
>> - Add ValueLayout changes
>> - Tweak support for array element var handle
>> - ... and 47 more:
>> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>
>> 151: static SymbolLookup loaderLookup() {
>> 152: Class caller = Reflection.getCallerClass();
>> 153: ClassLoader loader =
>> Objects.requireNonNull(caller.getClassLoader());
>
> Shouldn’t this be changed to throw `IllegalCallerException` instead of
> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
> `null`[^1] or when `caller` itself is `null`[^2]?
>
> [^1]: This happens when `caller` is on the bootstrap classpath.
> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
> code and no **Java** code is on the call stack.
Good points. Regarding `ClassLoader` being null, I think we can still return
something using the `BootLoader`'s `NativeLibraries` object - that would allow
this method to be called internally. @mlchung Can you please confirm?
As for the caller sensitive having no real caller (e.g. because method is
called directly from native code), I think we should add a check in
`Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
check does not compromise upcall stub created via the Linker interface (e.g. a
Java upcall might require to perform restricted operations - but those
operation always happen inside the upcall MH, which is always associated with
some class).
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Tue, 3 May 2022 10:40:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 57 commits:
>
> - Merge branch 'master' into foreign-preview
> - Update
> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Tweak support for Linker lookup
>Improve javadoc of SymbolLookup
> - Tweak Addressable javadoc
> - Downcall and upcall tests use wrong layout which is missing some trailing
> padding
> - Simplify libraryLookup impl
> - Address CSR comments
> - Merge branch 'master' into foreign-preview
> - Add ValueLayout changes
> - Tweak support for array element var handle
> - ... and 47 more:
> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 116:
> 114: * Linker nativeLinker = Linker.nativeLinker();
> 115: * SymbolLookup stdlib = nativeLinker.defaultLookup();
> 116: * MemorySegment malloc = stdlib.lookup("malloc");
This should be:
Suggestion:
* Optional malloc = stdlib.lookup("malloc");
or
Suggestion:
* MemorySegment malloc = stdlib.lookup("malloc").orElseThrow();
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
> 151: static SymbolLookup loaderLookup() {
> 152: Class caller = Reflection.getCallerClass();
> 153: ClassLoader loader =
> Objects.requireNonNull(caller.getClassLoader());
Shouldn’t this be changed to throw `IllegalCallerException` instead of throwing
`NullPointerException` when the `caller`’s `ClassLoader` is `null`[^1] or when
`caller` itself is `null`[^2]?
[^1]: This happens when `caller` is on the bootstrap classpath.
[^2]: This happens when `SymbolLookup.loaderLookup()` is called by native code
and no **Java** code is on the call stack.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
> This PR contains the API and implementation changes for JEP-424 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits: - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Tweak Addressable javadoc - Downcall and upcall tests use wrong layout which is missing some trailing padding - Simplify libraryLookup impl - Address CSR comments - Merge branch 'master' into foreign-preview - Add ValueLayout changes - Tweak support for array element var handle - ... and 47 more: https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7888=36 Stats: 65464 lines in 367 files changed: 43470 ins; 19432 del; 2562 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
