Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-12 Thread Maurizio Cimadamore
On Wed, 11 Nov 2020 14:18:33 GMT, Vladimir Ivanov  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #7 from JornVernee/Additional_Review_Comments
>>
>>Additional review comments
>>  - Revert System.java changes
>>  - Set copyright year for added files to 2020
>>  - Check result of AttachCurrentThread
>>  - Sort includes alphabetically
>>  - Relax ret_addr_offset() assert
>>  - Extra space after if
>>  - remove excessive asserts in ProgrammableInvoker::invoke_native
>>  - Remove os::is_MP() check
>>  - remove blank line in thread.hpp
>
> I made a pass over hotspot code. Overall, it looks good. Some comments follow.

I've just merged against master - which now contains the foreign memory API 
changes that this JEP depends on. I believe reviewing the changes should now be 
easier, as only the relevant changes should be presented in the "File Changed" 
tab.

-

PR: https://git.openjdk.java.net/jdk/pull/634


Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread Jorn Vernee
On Wed, 11 Nov 2020 11:05:47 GMT, Vladimir Ivanov  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #7 from JornVernee/Additional_Review_Comments
>>
>>Additional review comments
>>  - Revert System.java changes
>>  - Set copyright year for added files to 2020
>>  - Check result of AttachCurrentThread
>>  - Sort includes alphabetically
>>  - Relax ret_addr_offset() assert
>>  - Extra space after if
>>  - remove excessive asserts in ProgrammableInvoker::invoke_native
>>  - Remove os::is_MP() check
>>  - remove blank line in thread.hpp
>
> src/hotspot/share/opto/machnode.cpp line 831:
> 
>> 829:   st->print("%s ",_name);
>> 830:   st->print("_arg_regs: ");
>> 831:   _arg_regs.print_on(st);
> 
> It doesn't print any useful info: `_arg_regs: 
> AllocatedObj(0x00011cf5cbe8)`. Please, improve it.

Ok, I added printing to GrowableArray at some point, but seems that this was 
removed in a merge maybe.

> src/hotspot/share/opto/graphKit.cpp line 2665:
> 
>> 2663: for (uint vm_ret_pos = 0; vm_ret_pos < n_returns; vm_ret_pos++) {
>> 2664:   if (new_call_type->range()->field_at(TypeFunc::Parms + 
>> vm_ret_pos)  == Type::HALF) {
>> 2665: // FIXME is this needed?
> 
> Why do you need the projection at all? Please, clarify and remove `FIXME` 
> comment.

Was a leftover. Will remove

> src/hotspot/share/opto/graphKit.cpp line 2675:
> 
>> 2673: // Unpack native results if needed
>> 2674: // Need this method type since it's unerased
>> 2675: switch (nep->method_type()->rtype()->basic_type()) {
> 
> Are calls returning multiple values supported right now? (From what I'm 
> seeing in other places, they are not supported.) If not, then you don't need 
> a loop over return values and there are other places where it can simplify 
> code.

Yes, multiple returns are not supported currently. Will simplify this.

-

PR: https://git.openjdk.java.net/jdk/pull/634


Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread Jorn Vernee
On Wed, 11 Nov 2020 14:17:20 GMT, Vladimir Ivanov  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #7 from JornVernee/Additional_Review_Comments
>>
>>Additional review comments
>>  - Revert System.java changes
>>  - Set copyright year for added files to 2020
>>  - Check result of AttachCurrentThread
>>  - Sort includes alphabetically
>>  - Relax ret_addr_offset() assert
>>  - Extra space after if
>>  - remove excessive asserts in ProgrammableInvoker::invoke_native
>>  - Remove os::is_MP() check
>>  - remove blank line in thread.hpp
>
> src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp line 77:
> 
>> 75: XMMRegister reg = _abi->_vector_argument_registers.at(i);
>> 76: size_t offs = _layout->arguments_vector + i * sizeof(VectorRegister);
>> 77: if (UseAVX >= 3) {
> 
> Assuming worst-case scenario (w.r.t. value size) is sub-optimal. Considering 
> FP values are handled as vectors you end up operating on 32-bit/64-bit values 
> as if they were 512-bit in size. And, in addition to wasted memory bandwidth, 
> EVEX-encoded instructions may trigger CPU frequency scaling which will 
> penalize cases when AVX512 is not used. So, it is worth considering 
> annotating vector values with their actual sizes and taking the size into 
> account when operating on vectors.

Yes, more cleanup is needed here. We don't support vectors at all right now, so 
I'd rather remove this code and only operate on XMM registers instead.

In the future this could be handled using separate VMStorage types for 
different vector sizes.

-

PR: https://git.openjdk.java.net/jdk/pull/634


Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread Jorn Vernee
On Wed, 11 Nov 2020 12:44:56 GMT, Vladimir Ivanov  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #7 from JornVernee/Additional_Review_Comments
>>
>>Additional review comments
>>  - Revert System.java changes
>>  - Set copyright year for added files to 2020
>>  - Check result of AttachCurrentThread
>>  - Sort includes alphabetically
>>  - Relax ret_addr_offset() assert
>>  - Extra space after if
>>  - remove excessive asserts in ProgrammableInvoker::invoke_native
>>  - Remove os::is_MP() check
>>  - remove blank line in thread.hpp
>
> src/hotspot/share/opto/callnode.cpp line 1143:
> 
>> 1141: case TypeFunc::Parms:
>> 1142: default: {
>> 1143:   if(tf()->range()->field_at(proj->_con) == Type::HALF) {
> 
> That's `TypeFunc::Parms+1` case in `CallNode::match`. Why did you decide to 
> move it to `default` case? 
> 
> Overall, it looks very similar to `CallNode::match`. Why not just customize 
> `OptoRegPair regs` computation for `CallNative` there?

For native calls we can have multiple return values, at least in theory. 
Currently this is not the case though.

Will take another look.

-

PR: https://git.openjdk.java.net/jdk/pull/634


Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread Vladimir Ivanov
On Tue, 10 Nov 2020 14:16:22 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 

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread David Holmes

On 11/11/2020 9:19 pm, Jorn Vernee wrote:

On Wed, 11 Nov 2020 07:18:33 GMT, David Holmes  wrote:


Maurizio Cimadamore has updated the pull request incrementally with 10 
additional commits since the last revision:

  - Merge pull request #7 from JornVernee/Additional_Review_Comments

Additional review comments

  - Revert System.java changes
  - Set copyright year for added files to 2020
  - Check result of AttachCurrentThread
  - Sort includes alphabetically
  - Relax ret_addr_offset() assert
  - Extra space after if
  - remove excessive asserts in ProgrammableInvoker::invoke_native
  - Remove os::is_MP() check
  - remove blank line in thread.hpp


src/hotspot/share/prims/universalUpcallHandler.cpp line 55:


53: JavaVM_ *vm = (JavaVM *)(&main_vm);
54: jint result = vm->functions->AttachCurrentThread(vm, (void**) &p_env, 
nullptr);
55: guarantee(result == JNI_OK, "Could not attach thread for upcall. JNI error 
code: %d", result);


I'm assuming you don't have a mechanism for conveying an error back to the 
original entry point used by the native thread? Attaching an existing thread 
should only fail if we run out of C-Heap, so we're on the brink of aborting 
anyway, but still the guarantee here is not ideal.


Yeah, we have no idea where/how to report such and error. The native code might 
just call a function through a function pointer like `void(*)(void)` i.e. no 
place to return an error code. Also, since it's a previously non-attached 
thread, there is no JavaFrameAnchor that gives us a last Java frame we could 
jump back to and throw an exception.


Yeah not a solvable problem when you can transparently attach to the VM.


Is there's a more explicit way to exit with an error, other than a `guarantee` 
that you would prefer here?


Perhaps vm_exit_out_if_memory under the assumption that is the only 
reason attach could fail. But really I don't think it makes much 
practical difference.


Thanks,
David
-


-

PR: https://git.openjdk.java.net/jdk/pull/634



Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-11 Thread Jorn Vernee
On Wed, 11 Nov 2020 07:18:33 GMT, David Holmes  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #7 from JornVernee/Additional_Review_Comments
>>
>>Additional review comments
>>  - Revert System.java changes
>>  - Set copyright year for added files to 2020
>>  - Check result of AttachCurrentThread
>>  - Sort includes alphabetically
>>  - Relax ret_addr_offset() assert
>>  - Extra space after if
>>  - remove excessive asserts in ProgrammableInvoker::invoke_native
>>  - Remove os::is_MP() check
>>  - remove blank line in thread.hpp
>
> src/hotspot/share/prims/universalUpcallHandler.cpp line 55:
> 
>> 53: JavaVM_ *vm = (JavaVM *)(&main_vm);
>> 54: jint result = vm->functions->AttachCurrentThread(vm, (void**) 
>> &p_env, nullptr);
>> 55: guarantee(result == JNI_OK, "Could not attach thread for upcall. JNI 
>> error code: %d", result);
> 
> I'm assuming you don't have a mechanism for conveying an error back to the 
> original entry point used by the native thread? Attaching an existing thread 
> should only fail if we run out of C-Heap, so we're on the brink of aborting 
> anyway, but still the guarantee here is not ideal.

Yeah, we have no idea where/how to report such and error. The native code might 
just call a function through a function pointer like `void(*)(void)` i.e. no 
place to return an error code. Also, since it's a previously non-attached 
thread, there is no JavaFrameAnchor that gives us a last Java frame we could 
jump back to and throw an exception.

Is there's a more explicit way to exit with an error, other than a `guarantee` 
that you would prefer here?

-

PR: https://git.openjdk.java.net/jdk/pull/634


Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-10 Thread David Holmes
On Tue, 10 Nov 2020 14:16:22 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 

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-10 Thread Coleen Phillimore
On Tue, 10 Nov 2020 14:16:22 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 

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

2020-11-10 Thread Maurizio Cimadamore
> 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 restrictions which apply to JNI library loading (e.g. same 
> library can