8268339: Upstream: 8267989: Exceptions thrown during upcalls should be handled #4396
Conversation
|
👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into |
|
@JornVernee The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
security
|
/label remove security |
|
/csr |
|
@JornVernee |
|
@JornVernee this pull request will not be integrated until the CSR request JDK-8268343 for issue JDK-8268339 has been approved. |
| String arch = System.getProperty("os.arch"); | ||
| String os = System.getProperty("os.name"); | ||
| String arch = privilegedGetProperty("os.arch"); | ||
| String os = privilegedGetProperty("os.name"); |
There was a problem hiding this comment.
These calls were causing problems for the test with a SecurityManager. They should have been using priviledgedGetProperty in the first place, so I've changed them here.
Webrevs
|
|
The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout Upstream_Excp_Hndl
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
JornVernee
force-pushed
the
Upstream_Excp_Hndl_2
branch
from
b495aae
to
8be3e09
Compare
|
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 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. |
| private void run(boolean useSpec) throws IOException, InterruptedException { | ||
| Process process = new ProcessBuilder() | ||
| .command( | ||
| Paths.get(Utils.TEST_JDK) |
There was a problem hiding this comment.
You might find jdk.test.lib.JDKToolLauncher useful.
There was a problem hiding this comment.
Thanks for the suggestion (forgot to reply earlier). I think I copied the current way to find java from another test. I'll look at using JDKToolLauncher next time.
There was a problem hiding this comment.
I think this approach makes sense given the native code cannot react to the exception, possibly resulting in undefined behavior.
We might be able to better check upcall handles if they declare they throw and reject them, but it's tricky to nail down the exact conditions, so better to defer and spend more time getting it right.
(Perhaps one day we might be able to reflect over code and do more detailed analysis.)
JornVernee
changed the title
|
@JornVernee This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 43 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
added
integrated
|
@JornVernee Since your change was applied there have been 48 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit ab01cb5. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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: #4395This patch adds uniform exception handling for exceptions thrown during FLA upcalls. Currently, these exceptions are either handled similarly to the VMs
CATCHmacro, 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: openjdk/panama-foreign#543
Thanks,
Jorn
Testing:
jdk_foreigntest suite. Tier 1-2 for the original patch.Thanks,
Jorn
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4396/head:pull/4396$ git checkout pull/4396Update a local copy of the PR:
$ git checkout pull/4396$ git pull https://git.openjdk.java.net/jdk pull/4396/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4396View PR using the GUI difftool:
$ git pr show -t 4396Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4396.diff