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

8265484: Fix up TRAPS usage in GenerateOopMap::compute_map and callers #3580

Closed
wants to merge 3 commits into from

Conversation

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Apr 19, 2021

If compute_map encounters malformed bytecode, or an OOM condition then it can raise a LinkageError to be thrown in regular JavaThreads, and this is reflected in the use of TRAPS in a couple of methods. But for non-JavaThreads, and compiler threads, the occurrence of such an error is treated as a fatal error and we abort the VM.

In addition, most of the callers of compute_map actually use the CATCH macro, which just clears the exception and will abort a non-product VM.

This use of TRAPS is a problem for the change to convert TRAPS to JavaThread, because these methods are called by non-JavaThreads. To address this we change compute_map to create, but not throw, the exception and return a bool indicating whether an error occurred, and the caller can then choose to throw the exception itself.

Testing: tiers 1-3

Thanks,
David


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8265484: Fix up TRAPS usage in GenerateOopMap::compute_map and callers

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3580/head:pull/3580
$ git checkout pull/3580

Update a local copy of the PR:
$ git checkout pull/3580
$ git pull https://git.openjdk.java.net/jdk pull/3580/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3580

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3580.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 19, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@dholmes-ora The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the hotspot label Apr 19, 2021
@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 19, 2021

/label remove hotspot
/label add hotspot-runtime
/label add hotspot-compiler

Loading

@openjdk openjdk bot removed the hotspot label Apr 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@dholmes-ora
The hotspot label was successfully removed.

Loading

@dholmes-ora dholmes-ora marked this pull request as ready for review Apr 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@dholmes-ora
The hotspot-runtime label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@dholmes-ora
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 19, 2021

Webrevs

Loading

iklam
iklam approved these changes Apr 21, 2021
Copy link
Member

@iklam iklam left a comment

LGTM.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 21, 2021

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

8265484: Fix up TRAPS usage in GenerateOopMap::compute_map and callers

Reviewed-by: iklam, dlong, coleenp

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 93 new commits pushed to the master branch:

  • a8ddbd1: 8265683: vmTestbase/nsk/jdb tests failed with "JDWP exit error AGENT_ERROR_INTERNAL(181)"
  • 7a55914: 8264196: Change link_and_cleanup_shared_classes(CATCH) to CHECK
  • b84f690: 8265793: Remove duplicate jtreg TEST.groups references for some client tests
  • 0e00598: 8265782: Bump bootjdk to jdk-17+19 on macosx-aarch64 at Oracle
  • e81baea: 8265786: ProblemList serviceability/sa/sadebugd/DisableRegistryTest.java on ZGC
  • ca0de26: 8265699: (bf) Scopes passed to ScopedMemoryAccess.copy[Swap]Memory in incorrect order
  • b930bb1: 8265461: G1: Forwarding pointer removal thread sizing
  • f834557: 8258915: Temporary buffer cleanup
  • 31d8a19: 8265105: gc/arguments/TestSelectDefaultGC.java fails when compiler1 is disabled
  • 657f103: 8057543: Replace javac's Filter with Predicate (and lambdas)
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/1ac25b8201479be51113ff657b9a87fa29a690e8...master

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 master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Apr 21, 2021
@dean-long
Copy link
Member

@dean-long dean-long commented Apr 21, 2021

If this gets backported to an older jdk where CATCH called ShouldNotReachHere() instead of assert(), then it will be a change in behavior, right?

What's the reason for moving the exception throwing into the caller? Is it necessary or just an optimization?

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 21, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Dean,

Thanks for looking at this.

On 21/04/2021 1:23 pm, Dean Long wrote:

On Tue, 20 Apr 2021 22:04:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

If compute_map encounters malformed bytecode, or an OOM condition then it can raise a LinkageError to be thrown in regular JavaThreads, and this is reflected in the use of TRAPS in a couple of methods. But for non-JavaThreads, and compiler threads, the occurrence of such an error is treated as a fatal error and we abort the VM.

In addition, most of the callers of compute_map actually use the CATCH macro, which just clears the exception and will abort a non-product VM.

This use of TRAPS is a problem for the change to convert TRAPS to JavaThread, because these methods are called by non-JavaThreads. To address this we change compute_map to create, but not throw, the exception and return a bool indicating whether an error occurred, and the caller can then choose to throw the exception itself.

Testing: tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Additional comments on failure modes

If this gets backported to an older jdk where CATCH called ShouldNotReachHere() instead of assert(), then it will be a change in behavior, right?

Right - though there is really no direct reason to backport such
cleanups. Also Ioi and I were discussing that perhaps we should revert
the current version of CATCH so it once again aborts in the product VM,
and then fix up call-sites like these. But these call-sites would
ideally properly deal with exceptions in regular JavaThreads - somehow.

What's the reason for moving the exception throwing into the caller? Is it necessary or just an optimization?

Necessary to get TRAPS out of the method signatures because I'm changing
TRAPS to be JavaThread (in another issue) and compute_map is not always
called on a JavaThread.

Thanks,
David

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 21, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Dean,

Thanks for looking at this.

On 21/04/2021 1:23 pm, Dean Long wrote:

On Tue, 20 Apr 2021 22:04:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

If compute_map encounters malformed bytecode, or an OOM condition then it can raise a LinkageError to be thrown in regular JavaThreads, and this is reflected in the use of TRAPS in a couple of methods. But for non-JavaThreads, and compiler threads, the occurrence of such an error is treated as a fatal error and we abort the VM.

In addition, most of the callers of compute_map actually use the CATCH macro, which just clears the exception and will abort a non-product VM.

This use of TRAPS is a problem for the change to convert TRAPS to JavaThread, because these methods are called by non-JavaThreads. To address this we change compute_map to create, but not throw, the exception and return a bool indicating whether an error occurred, and the caller can then choose to throw the exception itself.

Testing: tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Additional comments on failure modes

If this gets backported to an older jdk where CATCH called ShouldNotReachHere() instead of assert(), then it will be a change in behavior, right?

Right - though there is really no direct reason to backport such
cleanups. Also Ioi and I were discussing that perhaps we should revert
the current version of CATCH so it once again aborts in the product VM,
and then fix up call-sites like these. But these call-sites would
ideally properly deal with exceptions in regular JavaThreads - somehow.

What's the reason for moving the exception throwing into the caller? Is it necessary or just an optimization?

Necessary to get TRAPS out of the method signatures because I'm changing
TRAPS to be JavaThread (in another issue) and compute_map is not always
called on a JavaThread.

Thanks,
David

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 21, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 21/04/2021 11:54 am, Ioi Lam wrote:

On Tue, 20 Apr 2021 22:04:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

If compute_map encounters malformed bytecode, or an OOM condition then it can raise a LinkageError to be thrown in regular JavaThreads, and this is reflected in the use of TRAPS in a couple of methods. But for non-JavaThreads, and compiler threads, the occurrence of such an error is treated as a fatal error and we abort the VM.

In addition, most of the callers of compute_map actually use the CATCH macro, which just clears the exception and will abort a non-product VM.

This use of TRAPS is a problem for the change to convert TRAPS to JavaThread, because these methods are called by non-JavaThreads. To address this we change compute_map to create, but not throw, the exception and return a bool indicating whether an error occurred, and the caller can then choose to throw the exception itself.

Testing: tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Additional comments on failure modes

LGTM.

Thanks Ioi!

David

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 21, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 21/04/2021 11:54 am, Ioi Lam wrote:

On Tue, 20 Apr 2021 22:04:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

If compute_map encounters malformed bytecode, or an OOM condition then it can raise a LinkageError to be thrown in regular JavaThreads, and this is reflected in the use of TRAPS in a couple of methods. But for non-JavaThreads, and compiler threads, the occurrence of such an error is treated as a fatal error and we abort the VM.

In addition, most of the callers of compute_map actually use the CATCH macro, which just clears the exception and will abort a non-product VM.

This use of TRAPS is a problem for the change to convert TRAPS to JavaThread, because these methods are called by non-JavaThreads. To address this we change compute_map to create, but not throw, the exception and return a bool indicating whether an error occurred, and the caller can then choose to throw the exception itself.

Testing: tiers 1-3

Thanks,
David

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Additional comments on failure modes

LGTM.

Thanks Ioi!

David

Loading

Copy link
Contributor

@coleenp coleenp left a comment

This looks good to me other than changing asserts to fatals.

Loading

if (!gpi.compute_map(THREAD)) {
// Failure is only possible for a resource-area OOM, or malformed bytecode
// with verification disabled.
assert(false, "Unexpected exception from compute_map");
Copy link
Contributor

@coleenp coleenp Apr 21, 2021

Choose a reason for hiding this comment

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

Since CATCH was only recently changed from fatal to assert, making these fatal() wouldn't be a change in behavior and that's what this should do.

Loading

if (!GenerateOopMap::compute_map(current)) {
// Failure is only possible for a resource-area OOM, or malformed bytecode
// with verification disabled.
assert(false, "Unexpected exception from compute_map");
Copy link
Contributor

@coleenp coleenp Apr 21, 2021

Choose a reason for hiding this comment

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

same here, and below, should be fatal.

Loading

After a number of people examined this code it was decided that the best approach was to re-instate
the call to fatal() in product builds, that would have been present in the CATCH macro, until recent
changes converted that to an assert(). The code here is not prepared to continue if compute_map fails
so we have to abort. Such failures are extremely unlikely (metaspace or resource-area OOM, or malformed
bytecode where verification has been disabled).
Copy link
Contributor

@coleenp coleenp left a comment

Good cleanup!

Loading

iklam
iklam approved these changes Apr 22, 2021
Copy link
Member

@iklam iklam left a comment

Latest version LGTM

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 22, 2021

Thanks @coleenp and @iklam for the re-reviews. @dean-long are you also okay with the latest version? It addresses your earlier concern.

Thanks,
David

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Apr 23, 2021

Thanks @coleenp and @iklam for the re-reviews. @dean-long are you also okay with the latest version? It addresses your earlier concern.

Thanks,
David

Yes, looks good!

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 23, 2021

/integrate

Loading

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Apr 23, 2021

Thanks @dean-long !

Loading

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

@openjdk openjdk bot commented Apr 23, 2021

@dholmes-ora Since your change was applied there have been 93 commits pushed to the master branch:

  • a8ddbd1: 8265683: vmTestbase/nsk/jdb tests failed with "JDWP exit error AGENT_ERROR_INTERNAL(181)"
  • 7a55914: 8264196: Change link_and_cleanup_shared_classes(CATCH) to CHECK
  • b84f690: 8265793: Remove duplicate jtreg TEST.groups references for some client tests
  • 0e00598: 8265782: Bump bootjdk to jdk-17+19 on macosx-aarch64 at Oracle
  • e81baea: 8265786: ProblemList serviceability/sa/sadebugd/DisableRegistryTest.java on ZGC
  • ca0de26: 8265699: (bf) Scopes passed to ScopedMemoryAccess.copy[Swap]Memory in incorrect order
  • b930bb1: 8265461: G1: Forwarding pointer removal thread sizing
  • f834557: 8258915: Temporary buffer cleanup
  • 31d8a19: 8265105: gc/arguments/TestSelectDefaultGC.java fails when compiler1 is disabled
  • 657f103: 8057543: Replace javac's Filter with Predicate (and lambdas)
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/1ac25b8201479be51113ff657b9a87fa29a690e8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 13d3263.

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

Loading

@dholmes-ora dholmes-ora deleted the 8265484 branch Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants