Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 17:08:28 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the first incubation round >> of the foreign linker access API incubation >> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory >> access support (see JEP 393 [2] and >> associated pull request [3]). >> The main goal of this API is to provide a way to call native functions from >> Java code without the need of intermediate >> JNI glue code. In order to do this, native calls are modeled through the >> MethodHandle API. I suggest reading the >> writeup [4] I put together few weeks ago, which illustrates what the foreign >> linker support is, and how it should be >> used by clients. Disclaimer: the pull request mechanism isn't great at >> managing *dependent* reviews. For this reasons, >> I'm attaching a webrev which contains only the differences between this PR >> and the memory access PR. I will be >> periodically uploading new webrevs, as new iterations come out, to try and >> make the life of reviewers as simple as >> possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the >> main architects of all the hotspot changes you >> see here, and without their help, the foreign linker support wouldn't be >> what it is today. As usual, a big thank to >> Paul Sandoz, who provided many insights (often by trying the bits first >> hand). Thanks Maurizio >> Webrev: >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff (relative to [3]): >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254232 >> >> >> >> ### API Changes >> >> The API changes are actually rather slim: >> >> * `LibraryLookup` >> * This class allows clients to lookup symbols in native libraries; the >> interface is fairly simple; you can load a library >> by name, or absolute path, and then lookup symbols on that library. >> * `FunctionDescriptor` >> * This is an abstraction that is very similar, in spirit, to `MethodType`; >> it is, at its core, an aggregate of memory >> layouts for the function arguments/return type. A function descriptor is >> used to describe the signature of a native >> function. >> * `CLinker` >> * This is the real star of the show. A `CLinker` has two main methods: >> `downcallHandle` and `upcallStub`; the first takes >> a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a >> `FunctionDescriptor` and returns a >> `MethodHandle` instance which can be used to call the target native >> symbol. The second takes an existing method handle, >> and a `FunctionDescriptor` and returns a new `MemorySegment` >> corresponding to a code stub allocated by the VM which >> acts as a trampoline from native code to the user-provided method >> handle. This is very useful for implementing upcalls. >>* This class also contains the various layout constants that should be >> used by clients when describing native signatures >> (e.g. `C_LONG` and friends); these layouts contain additional ABI >> classfication information (in the form of layout >> attributes) which is used by the runtime to *infer* how Java arguments >> should be shuffled for the native call to take >> place. >> * Finally, this class provides some helper functions e.g. so that clients >> can convert Java strings into C strings and >> back. >> * `NativeScope` >> * This is an helper class which allows clients to group together logically >> related allocations; that is, rather than >> allocating separate memory segments using separate *try-with-resource* >> constructs, a `NativeScope` allows clients to >> use a _single_ block, and allocate all the required segments there. This >> is not only an usability boost, but also a >> performance boost, since not all allocation requests will be turned into >> `malloc` calls. >> * `MemorySegment` >> * Only one method added here - namely `handoff(NativeScope)` which allows >> a segment to be transferred onto an existing >> native scope. >> >> ### Safety >> >> The foreign linker API is intrinsically unsafe; many things can go wrong >> when requesting a native method handle. For >> instance, the description of the native signature might be wrong (e.g. have >> too many arguments) - and the runtime has, >> in the general case, no way to detect such mismatches. For these reasons, >> obtaining a `CLinker` instance is >> a *restricted* operation, which can be enabled by specifying the usual JDK >> property `-Dforeign.restricted=permit` (as >> it's the case for other restricted method in the foreign memory API). ### >> Implementation changes The Java changes >> associated with `LibraryLookup` are relative
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v3]
On Thu, 15 Oct 2020 17:24:56 GMT, Stuart Monteith wrote:
>> Bernhard Urban-Forster has updated the pull request with a new target base
>> due to a merge or a rebase. The pull request
>> now contains 20 commits:
>> - disable warning only for hotspot
>> - Merge remote-tracking branch 'upstream/master' into
>> 8254072-fix-windows-arm64-warnings
>> - Merge remote-tracking branch 'upstream/master' into
>> 8254072-fix-windows-arm64-warnings
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
>> 'argument':
>>conversion from 'size_t' to 'int', possible loss of data
>> - Revert changes for "warning C4146: unary minus operator applied to
>> unsigned type, result still unsigned"
>> - msvc: disable unary minus warning for unsigned types
>> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'int', possible loss of data
>>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'const int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'unsigned int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146:
>> unary minus
>>operator applied to unsigned type, result still unsigned
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
>>warning C4267: 'argument': conversion from 'size_t' to 'int', possible
>> loss of data
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
>> 'type cast': conversion from 'unsigned int'
>>to 'address' of greater size
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> - ... and 10 more:
>> https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da
>
> src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp line 658:
>
>> 656: }
>> 657: }
>> 658: size_t size_in_bytes() { return 1ull << size(); }
>
> Capital ULL - I find that easer to search for and it is more consistent.
Thank you! Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Thu, 15 Oct 2020 09:57:14 GMT, Andrew Haley wrote:
> Fine, but please assert JavaThread::stack_shadow_zone_size() ==
> (int)JavaThread::stack_shadow_zone_size().
Done.
> Adding casts to shut up compilers is a very risky business, because often (if
> not in this case) the programmer doesn't
> understand the code well, and sprinkles casts everywhere. But casts disable
> compile-time type checking, and this leads
> to risks for future maintainability.
Full ACK and I appreciate your comments on this!
> I wonder if we should fix it in a better way, and use something like
> this in the future for all compiler warnings:
>
> ```
> template
> T1 checked_cast(T2 thing) {
> T1 result = static_cast(thing);
> guarantee(static_cast(result) == thing, "must be");
> return result;
> }
> ```
>
> I know this is additional work, but I promise we'll never need to have this
> conversation again.
This sounds like a great idea to me. I assume it doesn't fit into the scope of
this PR, therefore I've created
[JDK-8254856](https://bugs.openjdk.java.net/browse/JDK-8254856) to track it.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v4]
> I organized this PR so that each commit contains the warning emitted by MSVC
> as commit message and its relevant fix.
>
> Verified on
> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
> still works. Just mentioning this here, because
> it's yet another toolchain (Xcode / clang) that needs to be kept happy
> [going
> forward](https://openjdk.java.net/jeps/391).
Bernhard Urban-Forster has updated the pull request incrementally with two
additional commits since the last revision:
- uppercase suffix
- add assert
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/530/files
- new: https://git.openjdk.java.net/jdk/pull/530/files/32e922da..901bbd48
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=530=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=530=02-03
Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/530.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/530/head:pull/530
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator)
On Tue, 13 Oct 2020 13:08:14 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-Dforeign.restricted=permit` (as > it's the case for other restricted method in the foreign memory API). ### > Implementation changes The Java changes > associated with `LibraryLookup` are relative straightforward; the only > interesting thing to note here is that library > loading does _not_ depend on class
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v3]
On Thu, 15 Oct 2020 08:57:27 GMT, Bernhard Urban-Forster
wrote:
>> I organized this PR so that each commit contains the warning emitted by MSVC
>> as commit message and its relevant fix.
>>
>> Verified on
>> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
>> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
>> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
>> still works. Just mentioning this here, because
>> it's yet another toolchain (Xcode / clang) that needs to be kept happy
>> [going
>> forward](https://openjdk.java.net/jeps/391).
>
> Bernhard Urban-Forster has updated the pull request with a new target base
> due to a merge or a rebase. The pull request
> now contains 20 commits:
> - disable warning only for hotspot
> - Merge remote-tracking branch 'upstream/master' into
> 8254072-fix-windows-arm64-warnings
> - Merge remote-tracking branch 'upstream/master' into
> 8254072-fix-windows-arm64-warnings
> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
> 'argument':
>conversion from 'size_t' to 'int', possible loss of data
> - Revert changes for "warning C4146: unary minus operator applied to
> unsigned type, result still unsigned"
> - msvc: disable unary minus warning for unsigned types
> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
> warning C4267: 'initializing': conversion
>from 'size_t' to 'int', possible loss of data
>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
> warning C4267: 'initializing': conversion
>from 'size_t' to 'const int', possible loss of data
> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
> 'argument': conversion from 'size_t' to
>'unsigned int', possible loss of data
>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146:
> unary minus
>operator applied to unsigned type, result still unsigned
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
>warning C4267: 'argument': conversion from 'size_t' to 'int', possible
> loss of data
> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
> 'type cast': conversion from 'unsigned int'
>to 'address' of greater size
> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da
src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp line 658:
> 656: }
> 657: }
> 658: size_t size_in_bytes() { return 1ull << size(); }
Capital ULL - I find that easer to search for and it is more consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-Dforeign.restricted=permit` (as > it's the case for other restricted method in the foreign memory API). ### > Implementation changes The Java changes > associated with `LibraryLookup` are relative straightforward; the only > interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v3]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-Dforeign.restricted=permit` (as > it's the case for other restricted method in the foreign memory API). ### > Implementation changes The Java changes > associated with `LibraryLookup` are relative straightforward; the only > interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v2]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-Dforeign.restricted=permit` (as > it's the case for other restricted method in the foreign memory API). ### > Implementation changes The Java changes > associated with `LibraryLookup` are relative straightforward; the only > interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same
RFR: 8254827: JVMCI: Enable it for Windows+AArch64
Use r18 as allocatable register on Linux only. A bootstrap works now (it has been crashing before due to r18 being allocated): $ ./windows-aarch64-server-fastdebug/bin/java.exe -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler -XX:+BootstrapJVMCI -version Bootstrapping JVMCI. in 17990 ms (compiled 3330 methods) openjdk version "16-internal" 2021-03-16 OpenJDK Runtime Environment (fastdebug build 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk) OpenJDK 64-Bit Server VM (fastdebug build 16-internal+0-adhoc.NORTHAMERICAbeurba.openjdk-jdk, mixed mode) Jtreg tests `test/hotspot/jtreg/compiler/jvmci` are passing as well. - Commit messages: - 8254827: JVMCI: Enable it for Windows+AArch64 Changes: https://git.openjdk.java.net/jdk/pull/685/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=685=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254827 Stats: 15 lines in 3 files changed: 8 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/685.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/685/head:pull/685 PR: https://git.openjdk.java.net/jdk/pull/685
Re: RFR: JDK-8252870: Finalize (remove "incubator" from) jpackage [v3]
> JDK-8252870: Finalize (remove "incubator" from) jpackage Andy Herrick has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into JDK-8252870 - JDK-8252870: Finalize (remove "incubator" from) jpackage - reverting two auto-generated files, and changing module-info to "@since 16" - JDK-8252870: Finalize (remove "incubator" from) jpackage Merge branch 'finalize' into JDK-8252870 - 8252869 Finalize (remove incubator from) jpackage (implementation) - Changes: https://git.openjdk.java.net/jdk/pull/633/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=633=02 Stats: 1733 lines in 259 files changed: 692 ins; 701 del; 340 mod Patch: https://git.openjdk.java.net/jdk/pull/633.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/633/head:pull/633 PR: https://git.openjdk.java.net/jdk/pull/633
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Thu, 15 Oct 2020 09:02:35 GMT, Bernhard Urban-Forster
wrote:
>> Changes requested by ihse (Reviewer).
>
> @theRealAph I prototyped changing the argument of `bang_stack_with_offset()`
> from `int` to `size_t` here:
> https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852.
> In that approach casting is
> essentially pushed down to `bang_stack_with_offset` because the assembler
> instruction of most (all) architectures that
> is eventually consuming that offset needs a signed integer anyway. Doesn't
> seem like a win to me to be honest. I would
> rather prefer to go with what we have in this patch (similar to what x86 is
> doing today):
> --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> @@ -1524,7 +1524,7 @@ nmethod*
> SharedRuntime::generate_native_wrapper(MacroAssembler* masm,
>
>// Generate stack overflow check
>if (UseStackBanging) {
> -__ bang_stack_with_offset(JavaThread::stack_shadow_zone_size());
> +__ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
>} else {
> Unimplemented();
>}
> and leave it with that. What do you think?
> @theRealAph I prototyped changing the argument of `bang_stack_with_offset()`
> from `int` to `size_t` here:
> [lewurm@85a8f65](https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852).
> In that approach
> casting is essentially pushed down to `bang_stack_with_offset` because the
> assembler instruction of most (all)
> architectures that is eventually consuming that offset needs a signed integer
> anyway. Doesn't seem like a win to me to
> be honest. I would rather prefer to go with what we have in this patch
> (similar to what x86 is doing today): ```diff
> --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> @@ -1524,7 +1524,7 @@ nmethod*
> SharedRuntime::generate_native_wrapper(MacroAssembler* masm,
>
>// Generate stack overflow check
>if (UseStackBanging) {
> -__ bang_stack_with_offset(JavaThread::stack_shadow_zone_size());
> +__ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
>} else {
> Unimplemented();
>}
> ```
>
> and leave it with that. What do you think?
Fine, but please assert `JavaThread::stack_shadow_zone_size() ==
(int)JavaThread::stack_shadow_zone_size()`.
If all this sounds a bit paranoid, that's because I am.
Adding casts to shut up compilers is a very risky business, because often (if
not in this case) the programmer doesn't
understand the code well, and just sprinkles casts everywhere. But those casts
disable compile-time type checking, and
this leads to risks for future maintainability.
I wonder if we should fix this in a better way, and use this in the future:
template
T1 checked_cast(T2 thing) {
guarantee(static_cast(thing) == thing, "must be");
return static_cast(thing);
}
Then I promise we'll never need to have this conversation again.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Mon, 12 Oct 2020 10:29:23 GMT, Magnus Ihse Bursie wrote:
>> Bernhard Urban-Forster has updated the pull request with a new target base
>> due to a merge or a rebase. The pull request
>> now contains 18 commits:
>> - Merge remote-tracking branch 'upstream/master' into
>> 8254072-fix-windows-arm64-warnings
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
>> 'argument':
>>conversion from 'size_t' to 'int', possible loss of data
>> - Revert changes for "warning C4146: unary minus operator applied to
>> unsigned type, result still unsigned"
>> - msvc: disable unary minus warning for unsigned types
>> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'int', possible loss of data
>>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'const int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'unsigned int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146:
>> unary minus
>>operator applied to unsigned type, result still unsigned
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
>>warning C4267: 'argument': conversion from 'size_t' to 'int', possible
>> loss of data
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
>> 'type cast': conversion from 'unsigned int'
>>to 'address' of greater size
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning
>> C4267: 'initializing': conversion from 'size_t' to
>>'int', possible loss of data
>>./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning
>> C4267: 'initializing': conversion from 'size_t' to
>>'const int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning
>> C4146: unary minus operator applied to unsigned
>>type, result still unsigned
>> - ... and 8 more:
>> https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4
>
> Changes requested by ihse (Reviewer).
@theRealAph I prototyped changing the argument of `bang_stack_with_offset()`
from `int` to `size_t` here:
https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852.
In that approach casting is
essentially pushed down to `bang_stack_with_offset` because the assembler
instruction of most (all) architectures that
is eventually consuming that offset needs a signed integer anyway. Doesn't seem
like a win to me to be honest.
I would rather prefer to go with what we have in this patch (similar to what
x86 is doing today):
--- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
@@ -1524,7 +1524,7 @@ nmethod*
SharedRuntime::generate_native_wrapper(MacroAssembler* masm,
// Generate stack overflow check
if (UseStackBanging) {
-__ bang_stack_with_offset(JavaThread::stack_shadow_zone_size());
+__ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
} else {
Unimplemented();
}
and leave it with that. What do you think?
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Mon, 12 Oct 2020 10:29:11 GMT, Magnus Ihse Bursie wrote: >> Bernhard Urban-Forster has updated the pull request with a new target base >> due to a merge or a rebase. The pull request >> now contains 18 commits: >> - Merge remote-tracking branch 'upstream/master' into >> 8254072-fix-windows-arm64-warnings >> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267: >> 'argument': >>conversion from 'size_t' to 'int', possible loss of data >> - Revert changes for "warning C4146: unary minus operator applied to >> unsigned type, result still unsigned" >> - msvc: disable unary minus warning for unsigned types >> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123): >> warning C4267: 'initializing': conversion >>from 'size_t' to 'int', possible loss of data >>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123): >> warning C4267: 'initializing': conversion >>from 'size_t' to 'const int', possible loss of data >> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267: >> 'argument': conversion from 'size_t' to >>'unsigned int', possible loss of data >>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146: >> unary minus >>operator applied to unsigned type, result still unsigned >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): >>warning C4267: 'argument': conversion from 'size_t' to 'int', possible >> loss of data >> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312: >> 'type cast': conversion from 'unsigned int' >>to 'address' of greater size >> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning >> C4267: 'initializing': conversion from 'size_t' to >>'int', possible loss of data >>./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning >> C4267: 'initializing': conversion from 'size_t' to >>'const int', possible loss of data >> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning >> C4146: unary minus operator applied to unsigned >>type, result still unsigned >> - ... and 8 more: >> https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4 > > make/autoconf/flags-cflags.m4 line 137: > >> 135: WARNINGS_ENABLE_ALL="-W3" >> 136: DISABLED_WARNINGS="4800" >> 137: DISABLED_WARNINGS+=" 4146" # unary minus operator applied to >> unsigned type, result still unsigned > > This change will affect *all* JDK code. I'm not sure this was intended? > > If it was intended, I think you need to motivate this more explicitly. > > If you only wanted to disable the warning for hotspot, the proper solution > would be to add it to > DISABLED_WARNINGS_microsoft in make/hotspot/lib/CompileJvm.gmk. Thank you @magicus! It was indeed meant only for the hotspot part. - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v3]
> I organized this PR so that each commit contains the warning emitted by MSVC
> as commit message and its relevant fix.
>
> Verified on
> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
> still works. Just mentioning this here, because
> it's yet another toolchain (Xcode / clang) that needs to be kept happy
> [going
> forward](https://openjdk.java.net/jeps/391).
Bernhard Urban-Forster has updated the pull request with a new target base due
to a merge or a rebase. The pull request
now contains 20 commits:
- disable warning only for hotspot
- Merge remote-tracking branch 'upstream/master' into
8254072-fix-windows-arm64-warnings
- Merge remote-tracking branch 'upstream/master' into
8254072-fix-windows-arm64-warnings
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
'argument':
conversion from 'size_t' to 'int', possible loss of data
- Revert changes for "warning C4146: unary minus operator applied to unsigned
type, result still unsigned"
- msvc: disable unary minus warning for unsigned types
- ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'int', possible loss of data
./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'const int', possible loss of data
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
'argument': conversion from 'size_t' to
'unsigned int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146: unary
minus
operator applied to unsigned type, result still unsigned
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss
of data
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
'type cast': conversion from 'unsigned int'
to 'address' of greater size
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
- ... and 10 more: https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da
-
Changes: https://git.openjdk.java.net/jdk/pull/530/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=530=02
Stats: 22 lines in 8 files changed: 1 ins; 0 del; 21 mod
Patch: https://git.openjdk.java.net/jdk/pull/530.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/530/head:pull/530
PR: https://git.openjdk.java.net/jdk/pull/530
