8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d #6900

coleenp · 2021-12-20T22:59:39Z

Thanks to @stefank and @fisk for the diagnosis. ZGC is looking at metadata in unloaded nmethods, but redefinition doesn't keep this metadata from being deallocated. Change the iterators that mark metadata in use to walk unloaded nmethods as well as other alive nmethods.

The test case reproduces the crash on windows if run in an 100 iteration loop. This fix does not crash in the same test. Also ran tier1-6.

Progress

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

Issue

JDK-8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d

Reviewers

Erik Österlund (@fisk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6900

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

Using diff file

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

…ith EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d

bridgekeeper · 2021-12-20T22:59:57Z

👋 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 · 2021-12-20T23:01:52Z

@coleenp The following label will be automatically applied to this pull request:

hotspot-compiler

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.

mlbridge · 2021-12-20T23:05:57Z

Webrevs

00: Full (75f36c15)

fisk

Looks good, thanks for fixing this!

openjdk · 2021-12-21T08:10:55Z

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

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d

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

ff5d417: 8278893: Parallel: Remove GCWorkerDelayMillis
5179672: 8278953: Clarify Class.getDeclaredConstructor specification
8549d8b: 8277100: Dynamic dump can inadvertently overwrite default CDS archive
4c78c9a: 8270929: Obsolete the FilterSpuriousWakeups flag in JDK 19
06206c7: 8278949: Cleanups for 8277850
31fbb87: 6462028: MaskFormatter API documentation refers to getDisplayValue
63e4303: 8278519: serviceability/jvmti/FieldAccessWatch/FieldAccessWatch.java failed "assert(handle != __null) failed: JNI handle should not be null"
6f0e8da: 8278871: [JVMCI] assert((uint)reason < 2* _trap_hist_limit) failed: oob
3c10b5d: 8278104: C1 should support the compiler directive 'BreakAtExecute'
cc44e13: 8278623: compiler/vectorapi/reshape/TestVectorCastAVX512.java after JDK-8259610
... and 1 more: https://git.openjdk.java.net/jdk/compare/022e4f0f1c4862315b34595d6df228a49f67cb2e...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.

coleenp · 2021-12-21T13:30:44Z

/label add hotspot-runtime

coleenp · 2021-12-21T13:31:09Z

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

openjdk · 2021-12-21T13:31:43Z

@coleenp
The hotspot-runtime label was successfully added.

openjdk · 2021-12-21T13:32:51Z

@coleenp
The hotspot-runtime label was successfully removed.

openjdk · 2021-12-21T13:34:03Z

@coleenp
The hotspot label was successfully added.

openjdk · 2021-12-21T13:35:21Z

@coleenp
The hotspot-compiler label was successfully removed.

coleenp · 2021-12-21T17:02:50Z

I should have made this change to the JDK 18 repository. Closing this one.

sspitsyn · 2021-12-21T22:18:41Z

src/hotspot/share/code/codeCache.cpp

@@ -678,7 +678,7 @@ void CodeCache::nmethods_do(void f(nmethod* nm)) {

 void CodeCache::metadata_do(MetadataClosure* f) {
  assert_locked_or_safepoint(CodeCache_lock);
-  NMethodIterator iter(NMethodIterator::only_alive_and_not_unloading);
+  NMethodIterator iter(NMethodIterator::only_alive);


The CodeCache::metadata_do is used in MetadataOnStackMark::MetadataOnStackMark.
Besides RedefineClasses the MetadataOnStackMark is also used in ClassLoaderDataGraph::walk_metadata_and_clean_metaspaces.
Is the change at L681 correct for ClassLoaderDataGraph::walk_metadata_and_clean_metaspacesas well?

Yes, in ClassLoaderDataGraph::walk_metadata_and_clean_metaspaces, it calls the version of MetadataOnStackMark that doesn't call CodeCache::metadata_do - it only walks the metadata that was saved from the full redefinition walk:

caller:
MetadataOnStackMark md_on_stack(walk_all_metadata, /redefinition_walk/false);

callee:
if (redefinition_walk) {
// We have to walk the whole code cache during redefinition.
CodeCache::metadata_do(&md_on_stack);
} else {
CodeCache::old_nmethods_do(&md_on_stack);
}

During redefinition, we have to walk the unloaded nmethods as well as the alive nmethods.

The nmethods in the table are removed when the unloaded nmethods are flushed so that the table doesn't contain stale entries.

sspitsyn

Coleen,
Thank you for fixing this issue!
I've posted one question. Other than that it looks good to me.
Thanks,
Serguei

coleenp · 2021-12-21T22:40:45Z

@sspitsyn Thanks for looking at this. Can you review the JDK 18 PR instead?

sspitsyn · 2021-12-21T22:54:24Z

You are right about MetadataOnStackMark.
Yes, I'll review the 18 PR instead.

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed w…

75f36c1

…ith EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d

openjdk bot added the rfr Pull request is ready for review label Dec 20, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 20, 2021

fisk approved these changes Dec 21, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 21, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 21, 2021

openjdk bot removed the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 21, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 21, 2021

openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 21, 2021

coleenp closed this Dec 21, 2021

coleenp mentioned this pull request Dec 21, 2021

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d openjdk/jdk18#63

Closed

3 tasks

sspitsyn reviewed Dec 21, 2021

View reviewed changes

coleenp deleted the redefx branch February 24, 2022 13:56

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d #6900

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d #6900

coleenp commented Dec 20, 2021 •

edited by openjdk bot

bridgekeeper bot commented Dec 20, 2021

openjdk bot commented Dec 20, 2021

mlbridge bot commented Dec 20, 2021

fisk left a comment

openjdk bot commented Dec 21, 2021 •

edited

coleenp commented Dec 21, 2021

coleenp commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

coleenp commented Dec 21, 2021

sspitsyn Dec 21, 2021

coleenp Dec 21, 2021

coleenp Dec 21, 2021

sspitsyn left a comment

coleenp commented Dec 21, 2021

sspitsyn commented Dec 21, 2021

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d #6900

8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d #6900

Conversation

coleenp commented Dec 20, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 20, 2021

openjdk bot commented Dec 20, 2021

mlbridge bot commented Dec 20, 2021

Webrevs

fisk left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 21, 2021 • edited

coleenp commented Dec 21, 2021

coleenp commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

openjdk bot commented Dec 21, 2021

coleenp commented Dec 21, 2021

sspitsyn Dec 21, 2021

Choose a reason for hiding this comment

coleenp Dec 21, 2021

Choose a reason for hiding this comment

coleenp Dec 21, 2021

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

coleenp commented Dec 21, 2021

sspitsyn commented Dec 21, 2021

coleenp commented Dec 20, 2021 •

edited by openjdk bot

openjdk bot commented Dec 21, 2021 •

edited