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 straightforward;
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&pr=530&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=530&range=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 loade
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 restriction
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 restriction
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 restriction
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&pr=685&range=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&pr=633&range=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&pr=530&range=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