RFR: 8268339: Upstream: 8267989: Exceptions thrown during upcalls should be handled (Pt. 2) [v3]

[Jorn Vernee]

On Tue, 8 Jun 2021 17:07:43 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This is part 2 of a two part upstreaming process of the patch mentioned in the subject line. The patch was split into 2 in order to document 2 separate specification changes that arose from a change in the implementation, with 2 separate CSRs. The first patch can be found here: https://github.com/openjdk/jdk/pull/4395
>> 
>> This patch adds uniform exception handling for exceptions thrown during FLA upcalls. Currently, these exceptions are either handled similarly to the VMs `CATCH` macro, or ignored after which control returns to unsuspecting native code, until control returns to Java, which will then handle the exception. The handling depends on the invocation mode.
>> 
>> Both of these are bad. The former because a stack trace is not printed and instead the VM exits with a fatal error. The latter is bad because an upcall completing abruptly and returning to native code which has no idea an exception occurred is unsafe, in the sense that invariants about the state of the program that the native code depends on might no longer hold.
>> 
>> This patch adds uniform exception handling that replaces both of these cases (see `SharedUtils::handleUncaughtException`), which prints the exception stack trace, and then unconditionally exits the VM, which is the only safe course of action at that point.
>> 
>> Exceptions thrown by upcalls should instead be handled during the upcall itself, for instance by translating the exception into an error code that is then returned to the native caller, which can deal with it appropriately.
>> 
>> See also the original review for panama-foreign: https://github.com/openjdk/panama-foreign/pull/543
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `jdk_foreign` test suite.
>> 
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Remove spurious import
>  - Pt2

During some offline discussion of the CSR for the first part of this upstreaming, it was decided to focus on the second part only, and address the first part later.

I've rebased this PR onto the master branch, and this has the effect that the `CLinker::upcallStub` method no longer does an eager check for exceptions, and I've updated the javadoc to reflect this.

I've also removed the security manager usage, which tries to block calls to System.exit, from the test, since the new version of the jtreg agent also tries to call System.exit it seems, and the test fails because of the SecurityManager it installs. While it's theoretically possible to inspect the stack to figure out if the jtreg agent is calling System.exit, and then give permission, with the complexity that this would introduce in the test, together with the fact that the SecurityManager is deprecated, and it being seemingly unlikely that a SecurityManager check would be added to the alternative API currently being used to shut down the VM, it seems that programmatically testing that we can exit the VM with a SecurityManager installed is not worth the effort. We already know that the current code works from earlier testing.

-------------

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