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

8268364: jmethod clearing should be done during unloading #4643

Closed
wants to merge 5 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Jun 30, 2021

This patch moves the jmethod clearing to ClassLoaderData::unload() but also adds a check to Method::checked_resolved_jmethod_id() to handle the case where ZGC may be unloading a class but not have gotten to ClassLoaderData::unload() yet. JVMTI will read a NULL method for checked_resolved_jmethod_id() in this case, and not get a Method that will shortly, or has already been reclaimed in the Metaspace destructor.
Since I was there, I also added Method::is_valid_method() check to checked_resolve_jmethod_id. I don't think it's expensive anymore but it could be added under DEBUG. Either way method->method_holder()->is_loader_alive() will crash if !is_valid_method so we should leave it. As I wrote in the related issues, the bogus Method may have been because of a previous set of bugs with post_compiled_method_load events.

Tested with tiers 1-6 on linux-x64-debug and 1-3 on windows-x64-debug.

Also ran vmTestbase/nsk/{jdi,jvmti} tests with VM_OPTIONS=-XX:+UseZGC -XX:ZCollectionInterval=0.01 -XX:ZFragmen
tationLimit=0


Progress

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

Issue

  • JDK-8268364: jmethod clearing should be done during unloading

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4643

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 30, 2021

👋 Welcome back coleenp! 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.

@openjdk openjdk bot added the rfr label Jun 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

@coleenp 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.

@openjdk openjdk bot added the hotspot label Jun 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 30, 2021

Webrevs

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Jul 1, 2021

Since this fix touches jmethodIDs, I think it would be a good idea to run
it through Mach5 Tier[1-7] (on all Oracle platforms). (Yes, I'm paranoid.)

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Just comments for now. I need to think about this more.

src/hotspot/share/oops/method.cpp Show resolved Hide resolved
if (o == NULL || o == JNIMethodBlock::_free_method || !((Metadata*)o)->is_method()) {
if (o == NULL || o == JNIMethodBlock::_free_method) {
Copy link
Member

@dcubed-ojdk dcubed-ojdk Jul 1, 2021

Choose a reason for hiding this comment

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

I need to think more about deleting the is_method() check.

Copy link
Contributor Author

@coleenp coleenp Jul 1, 2021

Choose a reason for hiding this comment

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

The is_method() check was from when JNIHandleBlocks contained both jmethodID's and jobjects. It's a leftover from permgen elimination. I should have mentioned that in the PR comments. Now the only things that can be put in the JNIMethodBlocks are Methods, which the compiler type-checks, so that's the only thing that can come out (unless corrupted).

Copy link
Member

@dcubed-ojdk dcubed-ojdk Jul 1, 2021

Choose a reason for hiding this comment

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

Thanks for the explanation.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Still just comments because I'm not clear on the one code path.

src/hotspot/share/oops/method.cpp Show resolved Hide resolved
fisk
fisk approved these changes Jul 1, 2021
Copy link
Contributor

@fisk fisk left a comment

I did wonder if this was an issue but thought that if you are racingly using a jmethodid that is uncontrollably being unloaded (i.e. you are not holding the class alive in any way at all), then you are using the APIs in a seemingly buggy way that may or may not work the way you expect it to. Nevertheless, bugging out without a crash does seem better, and it did annoy me that these jmethodids are the only things unlinked that late.
Thanks for fixing this properly. This is the way I thought about fixing it, when I was not sure if it needed fixing or not. Sounds like it did need fixing after all.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

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

8268364: jmethod clearing should be done during unloading

Reviewed-by: dcubed, eosterlund

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

  • de61328: 8225559: assertion error at TransTypes.visitApply
  • 82bfc5d: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values
  • 18f356a: 8267307: Introduce new client property for XAWT: xawt.mwm_decor_title
  • 54a9c3e: 8133873: Simplify {Register,Unregister}NMethodOopClosure
  • 06d2620: 8268298: jdk/jfr/api/consumer/log/TestVerbosity.java fails: unexpected log message
  • d89e630: 8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block
  • 4660f72: 8268870: Remove dead code in metaspaceShared
  • 9def3b0: Merge
  • 9ac63a6: 8262841: Clarify the behavior of PhantomReference::refersTo
  • aba6c55: 8269703: ProblemList vmTestbase/nsk/jvmti/scenarios/sampling/SP07/sp07t002/TestDescription.java on Windows-X64 with -Xcomp
  • ... and 14 more: https://git.openjdk.java.net/jdk/compare/5c08344b646b92f3357a0daf32e77b21da5859ec...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.

@openjdk openjdk bot added the ready label Jul 1, 2021
Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thanks for the explanation on the is_loader_alive() part
and thanks for adding the new comment.

Thumbs up.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 1, 2021

Thanks Dan for the careful review and questions.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

So IIUC by clearing during unload, rather than in the destructor, it ensures that a jmethodID seen as valid by Method::checked_resolve_jmethod_id can't become invalid (due to a loader no longer being alive and being reclaimed) unless there is a safepoint (and assuming the JVM TI agent or application code, is not keeping the loader alive). So this fixes the situation in jvmti_GetMethodDeclaringClass as per the bug report, but in general we would have to be careful in the JVM TI implementation to avoid safepoints after validating the jmethodID. Is that correct?

Thanks,
David

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 2, 2021

I'm not sure why the bot didn't put my reply to @dholmes-ora in the PR.
For the record, all of the tier1-7 tests passed on the oracle supported platforms (linux-aarch64, linux-x64, windows-x64, macos-x64, and macos-aarch64 and their debug counterparts).
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 2, 2021

Going to push as commit 3d84398.
Since your change was applied there have been 42 commits pushed to the master branch:

  • 53ad903: 8269135: TestDifferentProtectionDomains runs into timeout in client VM
  • f8bcbf0: 8269596: Snapshot soft ref policy before marking/copying
  • 4107dcf: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads
  • 2baf498: 8269743: test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/stress/jni/nativeAndMH/Test.java crash with small heap (-Xmx50m)
  • 589f084: 8269110: ZGC: Remove dead code in zBarrier
  • b0e1867: Merge
  • a4d2a9a: 8269745: [JVMCI] restore original qualified exports to Graal
  • e377397: 8268566: java/foreign/TestResourceScope.java timed out
  • 6c76e77: 8260684: vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java timed out
  • 4bbf11d: 8269580: assert(is_valid()) failed: invalid register (-1)
  • ... and 32 more: https://git.openjdk.java.net/jdk/compare/5c08344b646b92f3357a0daf32e77b21da5859ec...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Jul 2, 2021

@coleenp Pushed as commit 3d84398.

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

@coleenp coleenp deleted the cld2 branch Jul 2, 2021
@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Jul 2, 2021

@coleenp - That was me that asked for the Tier[1-7]. Thanks for running that.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Jul 2, 2021

Thanks for the suggestion, Dan!

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 8, 2021

Mailing list message from Coleen Phillimore on hotspot-dev:

On 7/1/21 12:38 PM, Erik ?sterlund wrote:

On Thu, 1 Jul 2021 15:50:26 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

This patch moves the jmethod clearing to ClassLoaderData::unload() but also adds a check to Method::checked_resolved_jmethod_id() to handle the case where ZGC may be unloading a class but not have gotten to ClassLoaderData::unload() yet. JVMTI will read a NULL method for checked_resolved_jmethod_id() in this case, and not get a Method that will shortly, or has already been reclaimed in the Metaspace destructor.
Since I was there, I also added Method::is_valid_method() check to checked_resolve_jmethod_id. I don't think it's expensive anymore but it could be added under DEBUG. Either way method->method_holder()->is_loader_alive() will crash if !is_valid_method so we should leave it. As I wrote in the related issues, the bogus Method may have been because of a previous set of bugs with post_compiled_method_load events.

Tested with tiers 1-6 on linux-x64-debug and 1-3 on windows-x64-debug.

Also ran vmTestbase/nsk/{jdi,jvmti} tests with VM_OPTIONS=-XX:+UseZGC -XX:ZCollectionInterval=0.01 -XX:ZFragmen
tationLimit=0
Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision:

- ooops fixed typo
- Add a comment about is_loader_alive.
I did wonder if this was an issue but thought that if you are racingly using a jmethodid that is uncontrollably being unloaded (i.e. you are not holding the class alive in any way at all), then you are using the APIs in a seemingly buggy way that may or may not work the way you expect it to. Nevertheless, bugging out without a crash does seem better, and it did annoy me that these jmethodids are the only things unlinked that late.
Thanks for fixing this properly. This is the way I thought about fixing it, when I was not sure if it needed fixing or not. Sounds like it did need fixing after all.

Erik, thank you for confirming my diagnosis of the problem and
solution.? Ioi had found this through code inspection and carefully
reading JVMTI together helped. There weren't any test failures, and in
fact, this bug is getting marked with noreg-hard.

Thanks!
Coleen

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 8, 2021

Mailing list message from Coleen Phillimore on hotspot-dev:

On 7/1/21 9:09 PM, David Holmes wrote:

On Thu, 1 Jul 2021 15:50:26 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

This patch moves the jmethod clearing to ClassLoaderData::unload() but also adds a check to Method::checked_resolved_jmethod_id() to handle the case where ZGC may be unloading a class but not have gotten to ClassLoaderData::unload() yet. JVMTI will read a NULL method for checked_resolved_jmethod_id() in this case, and not get a Method that will shortly, or has already been reclaimed in the Metaspace destructor.
Since I was there, I also added Method::is_valid_method() check to checked_resolve_jmethod_id. I don't think it's expensive anymore but it could be added under DEBUG. Either way method->method_holder()->is_loader_alive() will crash if !is_valid_method so we should leave it. As I wrote in the related issues, the bogus Method may have been because of a previous set of bugs with post_compiled_method_load events.

Tested with tiers 1-6 on linux-x64-debug and 1-3 on windows-x64-debug.

Also ran vmTestbase/nsk/{jdi,jvmti} tests with VM_OPTIONS=-XX:+UseZGC -XX:ZCollectionInterval=0.01 -XX:ZFragmen
tationLimit=0
Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision:

- ooops fixed typo
- Add a comment about is_loader_alive.
Hi Coleen,

So IIUC by clearing during unload, rather than in the destructor, it ensures that a jmethodID seen as valid by `Method::checked_resolve_jmethod_id` can't become invalid (due to a loader no longer being alive and being reclaimed) unless there is a safepoint (and assuming the JVM TI agent or application code, is not keeping the loader alive). So this fixes the situation in `jvmti_GetMethodDeclaringClass` as per the bug report, but in general we would have to be careful in the JVM TI implementation to avoid safepoints after validating the jmethodID. Is that correct?

Yes, it is true.? Once you have a Method*, you'd have to have a
java.lang.Class object for the InstanceKlass on the stack (or a jobject
to one) if you want to use the Method* later.? This is okay for most
JVMTI entries because there is already a jobject pointing to the mirror,
and jvmti_GetMethodDeclaringClass was sort of the exception that didn't
have the mirror until after the call.
In general if you don't want your Method* to go away, you also need to
have a methodHandle so that redefinition doesn't clean it up if it's
redefined, which is a much less frequent but bad situation.
Coleen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
4 participants