Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267989: Exceptions thrown during upcalls should be handled #543

Conversation

@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 31, 2021

Hi,

This patch regularizes exception handling for exceptions thrown during upcalls. Exceptions thrown during upcalls are now always handled by printing out the stack trace and then calling System::exit (see the JBS issue for some motivation).

I've added some documentation for the exception handling to CLinker::upcallStub, as well as a new public int constant in CLinker which is the error code that is passed to System::exit. The returned error code can also be configured by a system property, which for now is mostly useful for testing purposes to make sure we don't get a consistent false positive.

Thanks,
Jorn


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issues

  • JDK-8267989: Exceptions thrown during upcalls should be handled
  • JDK-8268031: VarHandle combinator check for exceptions is too strict

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/543/head:pull/543
$ git checkout pull/543

Update a local copy of the PR:
$ git checkout pull/543
$ git pull https://git.openjdk.java.net/panama-foreign pull/543/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 543

View PR using the GUI difftool:
$ git pr show -t 543

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/543.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 31, 2021

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 31, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 31, 2021

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented May 31, 2021

@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:

8267989: Exceptions thrown during upcalls should be handled
8268031: VarHandle combinator check for exceptions is too strict

Reviewed-by: mcimadamore

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 no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 31, 2021
* the corresponding native stub will be deallocated.
* <p>
* Any exceptions that occur during an upcall should be handled during the upcall. The target method handle
Copy link
Collaborator

@mcimadamore mcimadamore May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is best to drop the first sentence, and go straight to the point, which is "The target method handle shoud not...".

Which reminds me, do we want to apply same logic we do for var handle combinators to detect if a method handle contains exceptions in its guts (by looking at signature of direct MH, or by looking for certain combinators).

Copy link
Member Author

@JornVernee JornVernee May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the first sentence, I'll see about adding a check for exceptions.

- tweak documentation
- remove system property
- simplify test & check stderr for stack trace message
- add security manager into test
- use privilegedGetProperty instead of System.getProperty in CABI
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jun 1, 2021

I've switched to using Shutdown.exit, and added a SecurityManager that blocks the normal System.exit call into the test. I've also switched CABI from using System.getProperty to determine the platform, to using priviledgedGetProperty, to avoid any security exceptions (found during testing).

I've removed the public ERR_UNCAUGHT_EXCEPTION constant, and changed the documentation to say that the VM will exit with a non-zero exit status in the case of an uncaught exception.

Finally, I've changed the test to always print the output as well, besides checking the contents.

I still have to add an eager check (and test) to see if the target of an upcall throws an exception.

Adapt the existing exception check done by MemoryHandles to be more lenient towards bound method handles which might or might not throw an exception, and re-use that check.
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jun 2, 2021

I've now added the eager exception checking to upcallStub.

As discussed offline, this reuses the exception check done for the VarHandle adapters, but makes the check more lenient towards BoundMetrhodHandles that might or might not throw an exception. The current check was too conservative toward rejecting such handles, because it rejects a BoundMehtodHandle that contained any MethodHandle field that could throw an exception. But, such an exception might be caught be an exclosing method handle. (see also https://bugs.openjdk.java.net/browse/JDK-8268031)

Instead, when a filter used for a var handle combinator throws a checked a exception when invoked, it is caught, and an IllegalStateException is thrown instead (with the original exception as the cause).

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks very good - thanks for fixing the VH adaptation code as well!

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jun 2, 2021

/solves 8268031

@openjdk
Copy link

@openjdk openjdk bot commented Jun 2, 2021

@JornVernee
Adding additional issue to solves list: 8268031: VarHandle combinator check for exceptions is too strict.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jun 2, 2021

/integrate

@openjdk openjdk bot closed this Jun 2, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 2, 2021

@JornVernee Pushed as commit d1b8b67.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants