Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-13 Thread Jorn Vernee
On Fri, 13 May 2022 19:13:46 GMT, Vladimir Ivanov  wrote:

>> Ok. Then, if no one objects, I will leave this area as-is for now. (and 
>> perhaps come back to this issue in the future, if it becomes more pressing).
>> 
>> (I'll also note that this issue already exists in the current code that's in 
>> mainline. So, it seems fair to address this as a followup as well, if 
>> needed).
>
> I don't see a way to reliably handle async exceptions purely with 
> `try/catch`. In the end, there's always a safepoint poll right before 
> returning from a method where new exception can be installed. So, there's 
> always a small chance present to observe a pending exception on VM side 
> irrespective of how hard you try on Java side. 
> 
> From reliability perspective, I find it important to gracefully handle such 
> corner cases. But I'm fine with addressing the problem separately.
> 
> As an alternative solution, exception handling for upcalls can be handled by 
> another upcall (into catch handler when pending exception is encountered). As 
> a bonus, it allows to handle repeated exceptions. In the worst case, it would 
> manifest as a hang (when async exceptions are continuously delivered). Still, 
> some special handling is needed for stack overflow errors. (Not sure how 
> those are handled now. Are they?)

SOE (of the Java exception kind) is not specially handled right now. I think 
the same rule applies there: we can't unwind or return to native frames (at 
least not without some guidance from the user).

I've filed an issue here to capture some of the discussion: 
https://bugs.openjdk.java.net/browse/JDK-8286761

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-13 Thread Vladimir Ivanov
On Thu, 12 May 2022 17:26:44 GMT, Jorn Vernee  wrote:

>>> Do nothing special for async exceptions. i.e. if they happen anywhere 
>>> between 1. and 6. they will end up in one of the fallback handlers and the 
>>> VM will be terminated.
>> 
>> My understanding is that if they materialize while we're executing the 
>> upcall Java code, if that code has a try/catch block, we will go there, 
>> rather than crash the VM.
>> 
>> In other words, IMHO the only problem with async exception is if they occur 
>> _after_ the Java user code has completed, because that will crash the Java 
>> adapter, this preventing it from returning to native call cleanly.
>> 
>> So, either we disable async exceptions during that phase (e.g. after user 
>> code has executed, but before we return back to native code), or we just 
>> punt and stop. Since this seems like a corner^3 case, and since there are 
>> also other issue with upcalls that can occur if other threads do not 
>> cooperate (e.g. an upcall can get stuck into an infinite safepoint if the VM 
>> exits while an async native thread runs the upcall), and given that 
>> obtaining a linker is a restricted operation anyway, I don't think we should 
>> bend over backwards to try to add 1% more safety to something that's 
>> unavoidably sharp anyways.
>
> Ok. Then, if no one objects, I will leave this area as-is for now. (and 
> perhaps come back to this issue in the future, if it becomes more pressing).
> 
> (I'll also note that this issue already exists in the current code that's in 
> mainline. So, it seems fair to address this as a followup as well, if needed).

I don't see a way to reliably handle async exceptions purely with `try/catch`. 
In the end, there's always a safepoint poll right before returning from a 
method where new exception can be installed. So, there's always a small chance 
present to observe a pending exception on VM side irrespective of how hard you 
try on Java side. 

>From reliability perspective, I find it important to gracefully handle such 
>corner cases. But I'm fine with addressing the problem separately.

As an alternative solution, exception handling for upcalls can be handled by 
another upcall (into catch handler when pending exception is encountered). As a 
bonus, it allows to handle repeated exceptions. In the worst case, it would 
manifest as a hang (when async exceptions are continuously delivered). Still, 
some special handling is needed for stack overflow errors. (Not sure how those 
are handled now. Are they?)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-12 Thread Jorn Vernee
On Thu, 12 May 2022 12:10:53 GMT, Maurizio Cimadamore  
wrote:

>> Okay, I see. I think I acted a little too hastily on this yesterday.  I'll 
>> revert the change that uses this blocking mechanism.
>> 
>> The stack more or less looks like this during an upcall:
>> 
>> 
>> | ---
>> |  |
>> | ---
>> | <1: user define try block with exception handler (maybe)> |
>> | ---
>> | <2: user code start> |
>> | ---
>> | <3: method handle impl frames 1> |
>> | ---
>> | <4: upcall wrapper class with fallback handler 1> |
>> | ---
>> | <5: method handle impl frames 2> |
>> | ---
>> | <6: upcallk stub with fallback handler 2> |
>> | <7: unknown native code>
>> | ---
>> |  |
>> 
>> 
>> I think there are several options to address async exceptions:
>> 1. Do nothing special for async exceptions. i.e. if they happen anywhere 
>> between 1. and 6. they will end up in one of the fallback handlers and the 
>> VM will be terminated.
>> 2. Block async exceptions in all code up from 6.
>> 3. Somehow only block async exceptions only between 6. and 1. 
>> I think that is possible by changing the API so that the user passes us 
>> a method handle to their fallback exception handler, and we invoke it in our 
>> code in 4. We would need 2 methods for blocking and unblocking async 
>> exceptions from Java. Then we could disable async exceptions at the start of 
>> 6. enabled them at the start of the try block in 4. (around the call to user 
>> code), and disable them at the end of this try block. Then finally re-enable 
>> them at the end of 6. If an exception occurs in the try block in 4., 
>> delegate to the user-defined exception handler (but use the VM terminate 
>> strategy as a fallback for when another exception occurs). The other problem 
>> I see with that is that to make that fast enough (i.e. not incur a ~1.5-2x 
>> cost on call overhead), we would need compiler intrinsics for the 
>> blocking/unblocking, and in the past we've been unable to define 'critical' 
>> sections of code like that in C2 (it's an unsolved problem at this point).
>
>> Do nothing special for async exceptions. i.e. if they happen anywhere 
>> between 1. and 6. they will end up in one of the fallback handlers and the 
>> VM will be terminated.
> 
> My understanding is that if they materialize while we're executing the upcall 
> Java code, if that code has a try/catch block, we will go there, rather than 
> crash the VM.
> 
> In other words, IMHO the only problem with async exception is if they occur 
> _after_ the Java user code has completed, because that will crash the Java 
> adapter, this preventing it from returning to native call cleanly.
> 
> So, either we disable async exceptions during that phase (e.g. after user 
> code has executed, but before we return back to native code), or we just punt 
> and stop. Since this seems like a corner^3 case, and since there are also 
> other issue with upcalls that can occur if other threads do not cooperate 
> (e.g. an upcall can get stuck into an infinite safepoint if the VM exits 
> while an async native thread runs the upcall), and given that obtaining a 
> linker is a restricted operation anyway, I don't think we should bend over 
> backwards to try to add 1% more safety to something that's unavoidably sharp 
> anyways.

Ok. Then, if no one objects, I will leave this area as-is for now. (and perhaps 
come back to this issue in the future, if it becomes more pressing).

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-12 Thread Maurizio Cimadamore
On Thu, 12 May 2022 09:24:23 GMT, Jorn Vernee  wrote:

> Do nothing special for async exceptions. i.e. if they happen anywhere between 
> 1. and 6. they will end up in one of the fallback handlers and the VM will be 
> terminated.

My understanding is that if they materialize while we're executing the upcall 
Java code, if that code has a try/catch block, we will go there, rather than 
crash the VM.

In other words, IMHO the only problem with async exception is if they occur 
_after_ the Java user code has completed, because that will crash the Java 
adapter, this preventing it from returning to native call cleanly.

So, either we disable async exceptions during that phase (e.g. after user code 
has executed, but before we return back to native code), or we just punt and 
stop. Since this seems like a corner^3 case, and since there are also other 
issue with upcalls that can occur if other threads do not cooperate (e.g. an 
upcall can get stuck into an infinite safepoint if the VM exits while an async 
native thread runs the upcall), and given that obtaining a linker is a 
restricted operation anyway, I don't think we should bend over backwards to try 
to add 1% more safety to something that's unavoidably sharp anyways.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-12 Thread Jorn Vernee
On Thu, 12 May 2022 03:32:15 GMT, David Holmes  wrote:

>> Is it possible for these upcalls to be nested? If yes, we could add a 
>> boolean to context to avoid unsetting the flag in those nested cases. And 
>> now that I think we should probably add that check in 
>> NoAsyncExceptionDeliveryMark too if we allow broader use of this flag. David 
>> added the NoAsyncExceptionDeliveryMark code with that assert about nesting 
>> so maybe he might have more insights about that.
>
> NoAsyncExceptionDeliveryMark is not for general use! There is no provision 
> for blocking async exceptions when running user-defined Java code. 
> NoAsyncExceptionDeliveryMark was purely for protecting "system Java code".

Okay, I see. I think I acted a little too hastily on this yesterday.  I'll 
revert the change that uses this blocking mechanism.

The stack more or less looks like this during an upcall:


| ---
|  |
| ---
| <1: user define try block with exception handler (maybe)> |
| ---
| <2: user code start> |
| ---
| <3: method handle impl frames 1> |
| ---
| <4: upcall wrapper class with fallback handler 1> |
| ---
| <5: method handle impl frames 2> |
| ---
| <6: upcallk stub with fallback handler 2> |
| <7: unknown native code>
| ---
|  |


I think there are several options to address async exceptions:
1. Do nothing special for async exceptions. i.e. if they happen anywhere 
between 1. and 6. they will up in one of the fallback handlers and the VM will 
be terminated.
2. Block async exceptions in all code up from 6.
3. Somehow only block async exceptions only between 6. and 1. 
I think that is possible by changing the API so that the user passes us a 
method handle to their fallback exception handler. We would need 2 methods for 
blocking and unblocking async exceptions from Java. Then we could disable async 
exceptions at the start of 6. enabled them at the start of the try block in 4. 
(around the call to user code), and disable them at the end of this try block. 
Then finally re-enable them at the end of 6. If an exception occurs in the try 
block in 4., delegate to the user-defined exception handler (but use the VM 
terminate strategy as a fallback for when another exception occurs). The other 
problem I see with that is that to make that fast enough (i.e. not incur a 
~1.5-2x cost on call overhead), we would need compiler intrinsics for the 
blocking/unblocking, and in the past we've been unable to define 'critical' 
sections of code like that in C2 (it's an unsolved problem at this point).

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread David Holmes
On Wed, 11 May 2022 20:27:42 GMT, Patricio Chilano Mateo 
 wrote:

>> I went ahead and implemented this suggestion. Now we block async exceptions 
>> in on_entry, and unblock in on_exit.
>
> Is it possible for these upcalls to be nested? If yes, we could add a boolean 
> to context to avoid unsetting the flag in those nested cases. And now that I 
> think we should probably add that check in NoAsyncExceptionDeliveryMark too 
> if we allow broader use of this flag. David added the 
> NoAsyncExceptionDeliveryMark code with that assert about nesting so maybe he 
> might have more insights about that.

NoAsyncExceptionDeliveryMark is not for general use! There is no provision for 
blocking async exceptions when running user-defined Java code. 
NoAsyncExceptionDeliveryMark was purely for protecting "system Java code".

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Patricio Chilano Mateo
On Wed, 11 May 2022 17:55:16 GMT, Jorn Vernee  wrote:

>> Oh nice! I was just thinking that the only possible way out of this 
>> conundrum would be to somehow block the delivery of async exceptions (at 
>> least outside of the user's exception handler). So, that seems to be exactly 
>> what we need :)
>
> I went ahead and implemented this suggestion. Now we block async exceptions 
> in on_entry, and unblock in on_exit.

Is it possible for these upcalls to be nested? If yes, we could add a boolean 
to context to avoid unsetting the flag in those nested cases. And now that I 
think we should probably add that check in NoAsyncExceptionDeliveryMark too if 
we allow broader use of this flag. David added the NoAsyncExceptionDeliveryMark 
code with that assert about nesting so maybe he might have more insights about 
that.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Wed, 11 May 2022 16:38:54 GMT, Jorn Vernee  wrote:

>> If you want to avoid processing asynchronous exceptions during this upcall 
>> you could block them (check NoAsyncExceptionDeliveryMark in 
>> JavaThread::exit()). Seems you could set the flag in 
>> ProgrammableUpcallhandler::on_entry() and unset it back on 
>> ProgrammableUpcallhandler::on_exit(). While that flag is set any 
>> asynchronous exception in the handshake queue of this thread will be skipped 
>> from processing. Maybe we should add a public method in the JavaThread 
>> class, block_async_exceptions()/unblock_async_exceptions() so we hide the 
>> handshake implementation.
>
> Oh nice! I was just thinking that the only possible way out of this conundrum 
> would be to somehow block the delivery of async exceptions (at least outside 
> of the user's exception handler). So, that seems to be exactly what we need :)

I went ahead and implemented this suggestion. Now we block async exceptions in 
on_entry, and unblock in on_exit.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Wed, 11 May 2022 16:20:32 GMT, Patricio Chilano Mateo 
 wrote:

>> Discussed this with Maurizio as well. 
>> 
>> My understanding is as follows:
>> 1. Async exceptions are installed into a thread's `pending_exception` field 
>> by handshake at a safepoint
>> 2. From there they are "thrown" (I guess during the same safepoint?), in 
>> which case we either end up in a user-defined exception handler (higher up 
>> the stack), in our Java fallback exception handler (print stack trace and 
>> terminate), or in this fallback exception handler (print and terminate).
>> 3. If we end up in this exception handler it means the async exception was 
>> installed somewhere outside of our Java exception handler and "thrown" from 
>> there. However, it also means that the Java code we were calling into 
>> completed abruptly.
>> 4. We've previously established that we have no way of signalling to the 
>> native code that is calling us that something went wrong, and so the only 
>> safe option is to terminate, as to not leave the application in an 
>> inconsistent state.
>> 
>> As a consequence, I don't think we have much choice in the case of async 
>> exceptions if we get here. Silently clearing them seems like it will leave 
>> the program in an inconsistent state (since we unwound some frames), so we 
>> have to terminate I think.
>> 
>> (@dholmes-ora is my understanding of async exceptions in point 1. and 2. 
>> correct here?)
>
> If you want to avoid processing asynchronous exceptions during this upcall 
> you could block them (check NoAsyncExceptionDeliveryMark in 
> JavaThread::exit()). Seems you could set the flag in 
> ProgrammableUpcallhandler::on_entry() and unset it back on 
> ProgrammableUpcallhandler::on_exit(). While that flag is set any asynchronous 
> exception in the handshake queue of this thread will be skipped from 
> processing. Maybe we should add a public method in the JavaThread class, 
> block_async_exceptions()/unblock_async_exceptions() so we hide the handshake 
> implementation.

Oh nice! I was just thinking that the only possible way out of this conundrum 
would be to somehow block the delivery of async exceptions (at least outside of 
the user's exception handler). So, that seems to be exactly what we need :)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Patricio Chilano Mateo
On Wed, 11 May 2022 15:44:19 GMT, Jorn Vernee  wrote:

>> We have an exception handler in Java as well, so this code is only a fail 
>> safe. But, I think in the case of asynchronous exceptions this might be 
>> problematic if the exception is discovered by the current thread outside of 
>> the Java exception handler, turned into a synchronous exception and then we 
>> get here and call `ProgrammableUpcallhandler::handle_uncaught_exception` and 
>> then crash. Or if the asynchronous exception is discovered in 
>> `ProgrammableUpcallHandler::on_exit` (where there is currently an assert for 
>> no exceptions).
>> 
>> I think you're right that, in both of those cases, if the exception is 
>> asynchronous, we should just ignore it.
>
> Discussed this with Maurizio as well. 
> 
> My understanding is as follows:
> 1. Async exceptions are installed into a thread's `pending_exception` field 
> by handshake at a safepoint
> 2. From there they are "thrown" (I guess during the same safepoint?), in 
> which case we either end up in a user-defined exception handler (higher up 
> the stack), in our Java fallback exception handler (print stack trace and 
> terminate), or in this fallback exception handler (print and terminate).
> 3. If we end up in this exception handler it means the async exception was 
> installed somewhere outside of our Java exception handler and "thrown" from 
> there. However, it also means that the Java code we were calling into 
> completed abruptly.
> 4. We've previously established that we have no way of signalling to the 
> native code that is calling us that something went wrong, and so the only 
> safe option is to terminate, as to not leave the application in an 
> inconsistent state.
> 
> As a consequence, I don't think we have much choice in the case of async 
> exceptions if we get here. Silently clearing them seems like it will leave 
> the program in an inconsistent state (since we unwound some frames), so we 
> have to terminate I think.
> 
> (@dholmes-ora is my understanding of async exceptions in point 1. and 2. 
> correct here?)

If you want to avoid processing asynchronous exceptions during this upcall you 
could block them (check NoAsyncExceptionDeliveryMark in JavaThread::exit()). 
Seems you could set the flag in ProgrammableUpcallhandler::on_entry() and unset 
it back on ProgrammableUpcallhandler::on_exit(). While that flag is set any 
asynchronous exception in the handshake queue of this thread will be skipped 
from processing. Maybe we should add a public method in the JavaThread class, 
block_async_exceptions()/unblock_async_exceptions() so we hide the handshake 
implementation.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Wed, 11 May 2022 12:05:29 GMT, Jorn Vernee  wrote:

>> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306:
>> 
>>> 304:   intptr_t exception_handler_offset = __ pc() - start;
>>> 305: 
>>> 306:   // Native caller has no idea how to handle exceptions,
>> 
>> Can you elaborate, please, how it is expected to work in presence of 
>> asynchronous exceptions? I'd expect to see a code which unconditionally 
>> clears pending exception with an assertion that verifies that the exception 
>> is of expected type.
>
> We have an exception handler in Java as well, so this code is only a fail 
> safe. But, I think in the case of asynchronous exceptions this might be 
> problematic if the exception is discovered by the current thread outside of 
> the Java exception handler, turned into a synchronous exception and then we 
> get here and call `ProgrammableUpcallhandler::handle_uncaught_exception` and 
> then crash. Or if the asynchronous exception is discovered in 
> `ProgrammableUpcallHandler::on_exit` (where there is currently an assert for 
> no exceptions).
> 
> I think you're right that, in both of those cases, if the exception is 
> asynchronous, we should just ignore it.

Discussed this with Maurizio as well. 

My understanding is as follows:
1. Async exceptions are installed into a thread's `pending_exception` field by 
handshake at a safepoint
2. From there they are "thrown" (I guess during the same safepoint?), in which 
case we either end up in a user-defined exception handler (higher up the 
stack), in our Java fallback exception handler (print stack trace and 
terminate), or in this fallback exception handler (print and terminate).
3. If we end up in this exception handler it means the async exception was 
installed somewhere outside of our Java exception handler and "thrown" from 
there. However, it also means that the Java code we were calling into completed 
abruptly.
4. We've previously established that we have no way of signalling to the native 
code that is calling us that something went wrong, and so the only safe option 
is to terminate, as to not leave the application in an inconsistent state.

As a consequence, I don't think we have much choice in the case of async 
exceptions if we get here. Silently clearing them seems like it will leave the 
program in an inconsistent state (since we unwound some frames), so we have to 
terminate I think.

(@dholmes-ora is my understanding of async exceptions in point 1. and 2. 
correct here?)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 20:38:05 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/prims/foreign_globals.cpp line 217:
> 
>> 215: 
>> 216:  public:
>> 217:   ForeignCMO(int total_in_args, const VMRegPair* in_regs, int 
>> total_out_args, VMRegPair* out_regs,
> 
> I propose to turn it into a trivial ctor and move all the logic into a helper 
> static function which returns the computed moves.

Done. Moved constructor logic into a `compute` method, and added a static 
helper that constructs a ComputeMoveOrder, calls `compute`, and then returns 
the moves.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Wed, 11 May 2022 11:06:51 GMT, Jorn Vernee  wrote:

>> src/hotspot/share/opto/callGenerator.cpp line 1131:
>> 
>>> 1129: 
>>> 1130: case vmIntrinsics::_linkToNative:
>>> 1131: print_inlining_failure(C, callee, jvms->depth() - 1, jvms->bci(),
>> 
>> Why is it unconditionally reported as inlining failure?
>
> The call that is being processed here is `linkToNative`, and that call is not 
> inlined, so reporting an inlining failure seems correct. We still go through 
> the method handle trampoline stub which loads the actual target from the 
> NativeEntryPoint appendix argument (`jump_to_native_invoker` in 
> methodHandles_x86.cpp).
> 
> It's potentially faster here to generate a runtime call to the underlying 
> invoker/downcall stub if the NativeEntryPoint is constant (i.e. avoid the 
> lookup through NEP in the MH trampoline), but I hadn't gotten to 
> investigating that yet.
> 
> From comparing the benchmark times between this and the old implementation 
> (which generated an inline call), they are not all that different. So it 
> seemed that doing something special here would not save that much time any 
> ways. (but, still would be good to investigate at some point)

I've filed: https://bugs.openjdk.java.net/browse/JDK-8286588

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 21:02:39 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> Nice work! Looks good. Some minor comments/questions follow.

@iwanowww Thanks for the review! I've addressed most comments already, but will 
have to think a bit on how to handle the asynchronous exceptions issue.

> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:
> 
>> 5529: }
>> 5530: 
>> 5531: // On 64 bit we will store integer like items to the stack as
> 
> Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`.

I've cleaned up the spaces and capitalization here a bit.

> src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 36:
> 
>> 34: #include "code/nativeInst.hpp"
>> 35: #include "code/vtableStubs.hpp"
>> 36: #include "compiler/disassembler.hpp"
> 
> Redundant includes? No new code added in the file.

Good catch. Seems like a merge artifact maybe. Removing them seems to be fine.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 20:53:37 GMT, Vladimir Ivanov  wrote:

>> src/hotspot/share/utilities/growableArray.hpp line 151:
>> 
>>> 149: return _data;
>>> 150:   }
>>> 151: 
>> 
>> This accessor is added to be able to temporarily view a stable GrowableArray 
>> instance as a C-style array. It is used to by `NativeCallConv` and 
>> `RegSpiller` in `foreign_globals.hpp`.
>> 
>> GrowableArray already has an `adr_at` accessor that does something similar, 
>> but using `adr_at(0)` fails on empty growable arrays since it also performs 
>> a bounds check. So it can not be used.
>
> Any problems with migrating `CallConv` and `RegSpiller`away from ` VMReg* + 
> int` to `GrowableArray`?

I'll try migrating to `GrowableArray`

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 20:45:02 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/cpu/x86/foreign_globals_x86_64.cpp line 52:
> 
>> 50: 
>> 51:   objArrayOop inputStorage = 
>> jdk_internal_foreign_abi_ABIDescriptor::inputStorage(abi_oop);
>> 52:   loadArray(inputStorage, INTEGER_TYPE, abi._integer_argument_registers, 
>> as_Register);
> 
> `loadArray` helper looks a bit misleading. In presence of `javaClass`-style 
> accessors, it misleadingly hints that it refers to some Java-level 
> operation/entity, though what it does it parses register list representation 
> backed by a Java array. I suggest to rename it to something like 
> `parse_argument_registers_array()`).

I went with `parse_register_array` (since it's also used for return registers)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Wed, 11 May 2022 10:51:10 GMT, Jorn Vernee  wrote:

>> src/hotspot/cpu/x86/foreign_globals_x86.hpp line 30:
>> 
>>> 28: #include "utilities/growableArray.hpp"
>>> 29: 
>>> 30: class outputStream;
>> 
>> Redundant declaration?
>
> Yeah, this whole file is redundant :) (replaced by foreign_globals_x86_64.hpp)

Hmm, it doesn't look like having x64 specific header files is support by the 
CPU_HEADER macros. Will stick to the shared header file for x86_32 and x86_64 
for now.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 21:01:48 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306:
> 
>> 304:   intptr_t exception_handler_offset = __ pc() - start;
>> 305: 
>> 306:   // Native caller has no idea how to handle exceptions,
> 
> Can you elaborate, please, how it is expected to work in presence of 
> asynchronous exceptions? I'd expect to see a code which unconditionally 
> clears pending exception with an assertion that verifies that the exception 
> is of expected type.

We have an exception handler in Java as well, so this code is only a fail safe. 
But, I think in the case of asynchronous exceptions this might be problematic 
if the exception is discovered by the current thread outside of the Java 
exception handler, turned into a synchronous exception and then we get here and 
call `ProgrammableUpcallhandler::handle_uncaught_exception` and then crash. Or 
if the asynchronous exception is discovered in 
`ProgrammableUpcallHandler::on_exit` (where there is currently an assert for no 
exceptions).

I think you're right that, in both of those cases, if the exception is 
asynchronous, we should just ignore it.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 20:48:47 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/prims/foreign_globals.hpp line 35:
> 
>> 33: #include CPU_HEADER(foreign_globals)
>> 34: 
>> 35: class CallConvClosure {
> 
> Just a question on terminology: why is it called a `Closure`?

It is the terminology used in other parts of hotspot for function objects it 
seems. See for instance the classes in `iterator.hpp`

> src/hotspot/share/prims/foreign_globals.hpp line 62:
> 
>> 60: 
>> 61: 
>> 62: class JavaCallConv : public CallConvClosure {
> 
> Does it really worth to abbreviate `CallingConvention` to `CallConv`?

Maybe not... I'll spell out the full thing.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 20:30:09 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/prims/foreign_globals.cpp line 147:
> 
>> 145: // based on ComputeMoveOrder from x86_64 shared runtime code.
>> 146: // with some changes.
>> 147: class ForeignCMO: public StackObj {
> 
> Considering how seldom it is used, I don't see much value in abbreviating it. 
> Also, the comment is misleading: there's no such entity as `ComputeMoveOrder` 
> in the code. And `compute_move_order` is completely removed by this change.

Good points, I think we can just rename this class to `ComputeMoveOrder` at 
this point.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 19:21:58 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/opto/callGenerator.cpp line 1131:
> 
>> 1129: 
>> 1130: case vmIntrinsics::_linkToNative:
>> 1131: print_inlining_failure(C, callee, jvms->depth() - 1, jvms->bci(),
> 
> Why is it unconditionally reported as inlining failure?

The call that is being processed here is `linkToNative`, and that call is not 
inlined, so reporting an inlining failure seems correct. We still go through 
the method handle trampoline stub which loads the actual target from the 
NativeEntryPoint (`jump_to_native_invoker` in methodHandles_x86.cpp).

It's potentially faster here to generate a runtime call to the underlying 
invoker/downcall stub if the NativeEntryPoint is constant (i.e. avoid the 
lookup through NEP in the MH trampoline), but I hadn't gotten to investigating 
that yet.

>From comparing the benchmark times between this and the old implementation 
>(which generated an inline call), they are not all that different. So it 
>seemed that doing something special here would not save that much time any 
>ways. (but, still would be good to investigate at some point)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 19:16:35 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/code/codeBlob.hpp line 754:
> 
>> 752: class ProgrammableUpcallHandler;
>> 753: 
>> 754: class OptimizedEntryBlob: public RuntimeBlob {
> 
> What's the motivation to move `OptimizedEntryBlob` up in the hierarchy (from 
> `BufferBlob` to `RuntimeBlob`)?

Some of it is discussed here: https://github.com/openjdk/panama-foreign/pull/617

Essentially, it is to avoid accidentally inheriting behavior from BufferBlob 
which we don't want. Also, BufferBlob currently expects a fixed-sized header 
(`sizeof(BufferBlob)`), while OptimizedEntryBlobs has fields, so we'd have to 
pass the real header size to the `BufferBlob` constructor, which is a bit messy.

It felt better to just cleanly break away from BufferBlob.

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 18:55:03 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/cpu/x86/foreign_globals_x86.hpp line 30:
> 
>> 28: #include "utilities/growableArray.hpp"
>> 29: 
>> 30: class outputStream;
> 
> Redundant declaration?

Yeah, this whole file is redundant :) (replaced by foreign_globals_x86_64.hpp)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 18:48:08 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5547:
> 
>> 5545:   } else if (dst.first()->is_stack()) {
>> 5546: // reg to stack
>> 5547: // Do we really have to sign extend???
> 
> Obsolete? Remove?

Yes, this looks like it can be removed. (was copied over from 
SharedRuntime_aarch64)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-11 Thread Jorn Vernee
On Tue, 10 May 2022 18:44:01 GMT, Vladimir Ivanov  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/cpu/aarch64/frame_aarch64.cpp line 379:
> 
>> 377:   // need unextended_sp here, since normal sp is wrong for interpreter 
>> callees
>> 378:   return reinterpret_cast(
>> 379: reinterpret_cast(frame.unextended_sp()) + 
>> in_bytes(_frame_data_offset));
> 
> Maybe use `address` instead of `char*`?

Ok. I think I used `char*` to try and avoid a potential strict-aliasing 
violation, but I don't think we compile with that turned on any ways.

Will change it to `address` (for x86 too)

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-10 Thread Vladimir Ivanov
On Mon, 28 Mar 2022 12:15:10 GMT, Jorn Vernee  wrote:

>> Jorn Vernee has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 21 commits:
>> 
>>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>>  - Remove unneeded ComputeMoveOrder
>>  - Remove comment about native calls in lcm.cpp
>>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>>
>>Reviewed-by: jvernee, mcimadamore
>>  - Update riscv and arm stubs
>>  - Remove spurious ProblemList change
>>  - Pass pointer to LogStream
>>  - Polish
>>  - Replace TraceNativeInvokers flag with unified logging
>>  - Fix other platforms, take 2
>>  - ... and 11 more: 
>> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91
>
> src/hotspot/share/utilities/growableArray.hpp line 151:
> 
>> 149: return _data;
>> 150:   }
>> 151: 
> 
> This accessor is added to be able to temporarily view a stable GrowableArray 
> instance as a C-style array. It is used to by `NativeCallConv` and 
> `RegSpiller` in `foreign_globals.hpp`.
> 
> GrowableArray already has an `adr_at` accessor that does something similar, 
> but using `adr_at(0)` fails on empty growable arrays since it also performs a 
> bounds check. So it can not be used.

Any problems with migrating `CallConv` and `RegSpiller`away from ` VMReg* + 
int` to `GrowableArray`?

-

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


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-10 Thread Vladimir Ivanov
On Mon, 9 May 2022 10:28:27 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR updates the VM implementation of the foreign linker, by bringing 
>> over commits from the panama-foreign repo.
>> 
>> This is split off from the main JEP integration for 19, since we have 
>> limited resources to handle this. As such, this PR might fall over to 20.
>> 
>> I've written up an overview of the Linker architecture here: 
>> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful 
>> to read that first.
>> 
>> This patch moves from the "legacy" implementation, to what is currently 
>> implemented in the panama-foreign repo, except for replacing the use of 
>> method handle combinators with ASM. That will come in a later path. To 
>> recap. This PR contains the following changes:
>> 
>> 1. VM stubs for downcalls are now generated up front, instead of lazily by 
>> C2 [1].
>> 2. the VM support for upcalls/downcalls now support all possible call 
>> shapes. And VM stubs and Java code implementing the buffered invocation 
>> strategy has been removed  [2], [3], [4], [5].
>> 3. The existing C2 intrinsification support for the `linkToNative` method 
>> handle linker was no longer needed and has been removed [6] (support might 
>> be re-added in another form later).
>> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now 
>> implements RuntimeBlob directly. Binding to java classes has been rewritten 
>> to use javaClasses.h/cpp (this wasn't previously possible due to these java 
>> classes being in an incubator module) [7], [8], [9].
>> 
>> While the patch mostly consists of VM changes, there are also some Java 
>> changes to support (2).
>> 
>> The original commit structure has been mostly retained, so it might be 
>> useful to look at a specific commit, or the corresponding patch in the 
>> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) 
>> repo as well. I've also left some inline comments to explain some of the 
>> changes, which will hopefully make reviewing easier.
>> 
>> Testing: Tier1-4
>> 
>> Thanks,
>> Jorn
>> 
>> [1]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
>> [2]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
>> [3]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
>> [4]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
>> [5]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
>> [6]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
>> [7]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
>> [8]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
>> [9]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9
>
> Jorn Vernee has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>  - Remove unneeded ComputeMoveOrder
>  - Remove comment about native calls in lcm.cpp
>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>
>Reviewed-by: jvernee, mcimadamore
>  - Update riscv and arm stubs
>  - Remove spurious ProblemList change
>  - Pass pointer to LogStream
>  - Polish
>  - Replace TraceNativeInvokers flag with unified logging
>  - Fix other platforms, take 2
>  - ... and 11 more: 
> https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91

Nice work! Looks good. Some minor comments/questions follow.

src/hotspot/cpu/aarch64/frame_aarch64.cpp line 379:

> 377:   // need unextended_sp here, since normal sp is wrong for interpreter 
> callees
> 378:   return reinterpret_cast(
> 379: reinterpret_cast(frame.unextended_sp()) + 
> in_bytes(_frame_data_offset));

Maybe use `address` instead of `char*`?

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:

> 5529: }
> 5530: 
> 5531: // On 64 bit we will store integer like items to the stack as

Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5547:

> 5545:   } else if (dst.first()->is_stack()) {
> 5546: // reg to stack
> 5547: // Do we really have to sign extend???

Obsolete? Remove?

src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306:

> 304:   intptr_t exception_handler_offset = __ pc() - start;
> 305: 
> 306:   // Native caller has no idea how to handle exceptions,

Can you elaborate, please, how it is expected to work in presence of 
asynchronous exceptions? I'd expect to see a code which unconditionally clears 
pending exception with an assertion that verifies that the exception is of 
expected type.


Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

2022-05-09 Thread Jorn Vernee
> Hi,
> 
> This PR updates the VM implementation of the foreign linker, by bringing over 
> commits from the panama-foreign repo.
> 
> This is split off from the main JEP integration for 19, since we have limited 
> resources to handle this. As such, this PR might fall over to 20.
> 
> I've written up an overview of the Linker architecture here: 
> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful 
> to read that first.
> 
> This patch moves from the "legacy" implementation, to what is currently 
> implemented in the panama-foreign repo, except for replacing the use of 
> method handle combinators with ASM. That will come in a later path. To recap. 
> This PR contains the following changes:
> 
> 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 
> [1].
> 2. the VM support for upcalls/downcalls now support all possible call shapes. 
> And VM stubs and Java code implementing the buffered invocation strategy has 
> been removed  [2], [3], [4], [5].
> 3. The existing C2 intrinsification support for the `linkToNative` method 
> handle linker was no longer needed and has been removed [6] (support might be 
> re-added in another form later).
> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now 
> implements RuntimeBlob directly. Binding to java classes has been rewritten 
> to use javaClasses.h/cpp (this wasn't previously possible due to these java 
> classes being in an incubator module) [7], [8], [9].
> 
> While the patch mostly consists of VM changes, there are also some Java 
> changes to support (2).
> 
> The original commit structure has been mostly retained, so it might be useful 
> to look at a specific commit, or the corresponding patch in the 
> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) 
> repo as well. I've also left some inline comments to explain some of the 
> changes, which will hopefully make reviewing easier.
> 
> Testing: Tier1-4
> 
> Thanks,
> Jorn
> 
> [1]: 
> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
> [2]: 
> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
> [3]: 
> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
> [4]: 
> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
> [5]: 
> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
> [6]: 
> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
> [7]: 
> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
> [8]: 
> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
> [9]: 
> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9

Jorn Vernee has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 21 commits:

 - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
 - Remove unneeded ComputeMoveOrder
 - Remove comment about native calls in lcm.cpp
 - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
   
   Reviewed-by: jvernee, mcimadamore
 - Update riscv and arm stubs
 - Remove spurious ProblemList change
 - Pass pointer to LogStream
 - Polish
 - Replace TraceNativeInvokers flag with unified logging
 - Fix other platforms, take 2
 - ... and 11 more: https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91

-

Changes: https://git.openjdk.java.net/jdk/pull/7959/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7959=06
  Stats: 6934 lines in 157 files changed: 2678 ins; 3218 del; 1038 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7959.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959

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