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

8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror" #584

Closed
wants to merge 3 commits into from

Conversation

zhengyu123
Copy link
Contributor

@zhengyu123 zhengyu123 commented Nov 4, 2021

Backport for parity with Oracle 11.0.15.

The original patch applies cleanly, but some of CDS tests fail due to following new assertion in ClassLoaderData::loaded_classes_do()

// To call this, one must have the MultiArray_lock held, but the _klasses list still has lock free reads. assert_locked_or_safepoint(MultiArray_lock);
The solution is to take MultiArray_lock in MetaspaceShared::link_and_cleanup_shared_classes() method

Test:

  • tier1

Progress

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

Issue

  • JDK-8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror"

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/584/head:pull/584
$ git checkout pull/584

Update a local copy of the PR:
$ git checkout pull/584
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/584/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 584

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/584.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 4, 2021

👋 Welcome back zgu! 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 changed the title Backport 172aed1a2d75756b140cb723133ac5fb67f7745e 8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror" Nov 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 4, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr labels Nov 4, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 4, 2021

Webrevs

Copy link
Member

@phohensee phohensee left a comment

I'd add the comment from JvmtiGetLoadedClasses::getLoadedClasses() before the added MultiArray_lock in metaspaceShared.cpp, vis

// To get a consistent list of classes we need MultiArray_lock to ensure
// array classes aren't created.

Otherwise, lgtm.

@zhengyu123
Copy link
Contributor Author

@zhengyu123 zhengyu123 commented Nov 5, 2021

I'd add the comment from JvmtiGetLoadedClasses::getLoadedClasses() before the added MultiArray_lock in metaspaceShared.cpp, vis

// To get a consistent list of classes we need MultiArray_lock to ensure
// array classes aren't created.

Otherwise, lgtm.

Thanks for the review, @phohensee . Added comment as you suggested

Copy link
Member

@phohensee phohensee left a comment

Thanks, Zhengyu. Lgtm.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2021

@zhengyu123 This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror"

Reviewed-by: phh

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

  • e929b18: 8272720: Fix the implementation of loop unrolling heuristic with LoopPercentProfileLimit
  • 5aa125e: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
  • 51390ac: 8233556: [TESTBUG] JPopupMenu tests fail on MacOS
  • a855b6f: 8015602: [macosx] Test javax/swing/SpringLayout/4726194/bug4726194.java fails on MacOSX
  • fe7ee62: 8223141: Change (count) suffix _ct into _cnt.
  • 1ae1563: 8267304: Bump global JTReg memory limit to 768m
  • ad40b61: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Nov 5, 2021
@zhengyu123
Copy link
Contributor Author

@zhengyu123 zhengyu123 commented Nov 6, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 6, 2021

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

  • 1d2fee0: 8233637: [TESTBUG] Swing ActionListenerCalledTwiceTest.java fails on macos
  • 8dc5f14: 8233641: [TESTBUG] JMenuItem test bug4171437.java fails on macos
  • ca35b75: 8233560: [TESTBUG] ToolTipManager/Test6256140.java is failing on macos
  • 73cbc72: 8248500: AArch64: Remove the r18 dependency on Windows AArch64
  • 889840b: 8233570: [TESTBUG] HTMLEditorKit test bug5043626.java is failing on macos
  • e929b18: 8272720: Fix the implementation of loop unrolling heuristic with LoopPercentProfileLimit
  • 5aa125e: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
  • 51390ac: 8233556: [TESTBUG] JPopupMenu tests fail on MacOS
  • a855b6f: 8015602: [macosx] Test javax/swing/SpringLayout/4726194/bug4726194.java fails on MacOSX
  • fe7ee62: 8223141: Change (count) suffix _ct into _cnt.
  • ... and 2 more: https://git.openjdk.java.net/jdk11u-dev/compare/36e54fbd936cebfa8e66483d0daa4d22dcc3100a...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Nov 6, 2021

@zhengyu123 Pushed as commit 3a35158.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 9, 2021

Mailing list message from Lindenmaier, Goetz on jdk-updates-dev:

Hi Zhengyu,

Since this change has been pushed, we see four test crashing in our CI,
see below. Do you have an idea what is wrong with these?

Best regards,
Goetz.

runtime/appcds/UnusedCPDuringDump.java
runtime/appcds/VerifierTest_1A.java
runtime/appcds/VerifierTest_1B.java
runtime/appcds/VerifierTest_2.java

# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/sapmnt/sapjvm_work/openjdk/nb/darwinintel64/jdk11u-dev/src/hotspot/share/runtime/javaCalls.cpp:60), pid=24040, tid=9987
# assert(!thread->owns_locks()) failed: must release all locks when leaving VM
#
# JRE version: OpenJDK Runtime Environment (11.0.14) (fastdebug build 11.0.14-internal+0-adhoc.openjdk.jdk11u-dev)
# Java VM: OpenJDK 64-Bit Server VM (fastdebug 11.0.14-internal+0-adhoc.openjdk.jdk11u-dev, interpreted mode, compressed oops, g1 gc, bsd-amd64)
# Core dump will be written. Default location: ./core.24040
#
# If you would like to submit a bug report, please visit:
# https://bugreport.java.com/bugreport/crash.jsp
#

--------------- S U M M A R Y ------------

Command Line: -Xmx768m -Djava.awt.headless=true -Djava.util.prefs.userRoot=/System/Volumes/Data/priv/jvmtests/output_openjdk11_dev_dbgU_darwinintel64/jtreg_hotspot_runtime_work/tmp -Djava.io.tmpdir=/System/Volumes/Data/priv/jvmtests/output_openjdk11_dev_dbgU_darwinintel64/jtreg_hotspot_runtime_work/tmp -Djdk.test.lib.artifacts.devkit-macosx_x64=/net/usr.work/jvmtests/devkit/Xcode9.2-MacOSX10.13-devkit -ea -esa -Xshare:dump -Xlog:cds,cds+hashtables -XX:ExtraSharedClassListFile=/System/Volumes/Data/priv/jvmtests/output_openjdk11_dev_dbgU_darwinintel64/jtreg_hotspot_runtime_work/JTwork/classes/runtime/appcds/UnusedCPDuringDump.d/runtime.appcds.UnusedCPDuringDump.java-test.classlist -XX:SharedArchiveFile=/System/Volumes/Data/priv/jvmtests/output_openjdk11_dev_dbgU_darwinintel64/jtreg_hotspot_runtime_work/JTwork/runtime/appcds/UnusedCPDuringDump/appcds-22h03m26s830.jsa -Xlog:class+path=info,class+load=debug

Host: wdfmacphy0129.wdf.global.corp.sap, MacPro7,1 x86_64 3200 MHz, 32 cores, 96G, Darwin 20.6.0
Time: Sun Nov 7 22:03:27 2021 CET elapsed time: 0.853748 seconds (0d 0h 0m 0s)

--------------- T H R E A D ---------------

Current thread (0x00007fe56f808620): JavaThread "main" [_thread_in_vm, id=9987, stack(0x0000700002ad5000,0x0000700002bd5000)]

Stack: [0x0000700002ad5000,0x0000700002bd5000], sp=0x0000700002bd2430, free space=1013k
Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.dylib+0xbe93f6] VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x6ba
V [libjvm.dylib+0xbe9b31] VMError::report_and_die(Thread*, void*, char const*, int, char const*, char const*, __va_list_tag*)+0x3f
V [libjvm.dylib+0x4470a3] report_vm_error(char const*, int, char const*, char const*, ...)+0xcc
V [libjvm.dylib+0x63c9c6] JavaCallWrapper::JavaCallWrapper(methodHandle const&, Handle, JavaValue*, Thread*)+0xce
V [libjvm.dylib+0x63e5bd] JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, Thread*)+0x387
V [libjvm.dylib+0x63d07d] JavaCalls::call_virtual(JavaValue*, Klass*, Symbol*, Symbol*, JavaCallArguments*, Thread*)+0x255
V [libjvm.dylib+0x63d382] JavaCalls::call_virtual(JavaValue*, Handle, Klass*, Symbol*, Symbol*, Handle, Thread*)+0xc6
V [libjvm.dylib+0xb4b17a] SystemDictionary::load_instance_class(Symbol*, Handle, Thread*)+0x156
V [libjvm.dylib+0xb49f67] SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle, Handle, Thread*)+0x6bd
V [libjvm.dylib+0xb4943d] SystemDictionary::resolve_or_fail(Symbol*, Handle, Handle, bool, Thread*)+0x1b
V [libjvm.dylib+0xbc0f34] VerificationType::resolve_and_check_assignability(InstanceKlass*, Symbol*, Symbol*, bool, bool, bool, Thread*)+0xa8
V [libjvm.dylib+0xbc11f2] VerificationType::is_reference_assignable_from(VerificationType const&, ClassVerifier*, bool, Thread*) const+0x12a
V [libjvm.dylib+0xbcfa3a] ClassVerifier::verify_invoke_instructions(RawBytecodeStream*, unsigned int, StackMapFrame*, bool, bool*, VerificationType, constantPoolHandle const&, StackMapTable*, Thread*)+0x86a
V [libjvm.dylib+0xbc40f3] ClassVerifier::verify_method(methodHandle const&, Thread*)+0x8cb
V [libjvm.dylib+0xbc233e] ClassVerifier::verify_class(Thread*)+0xa0
V [libjvm.dylib+0xbc1eba] Verifier::verify(InstanceKlass*, Verifier::Mode, bool, Thread*)+0x1f6
V [libjvm.dylib+0x617788] InstanceKlass::link_class_impl(bool, Thread*)+0x2ac
V [libjvm.dylib+0x6185ea] InstanceKlass::link_class(Thread*)+0x6e
V [libjvm.dylib+0x97f8e5] MetaspaceShared::try_link_class(InstanceKlass*, Thread*)+0xc5
V [libjvm.dylib+0x981a2a] LinkSharedClassesClosure::do_klass(Klass*)+0x4a
V [libjvm.dylib+0x369ff8] ClassLoaderData::loaded_classes_do(KlassClosure*)+0x16a
V [libjvm.dylib+0x36cc75] ClassLoaderDataGraph::loaded_classes_do(KlassClosure*)+0xad
V [libjvm.dylib+0x97f2af] MetaspaceShared::link_and_cleanup_shared_classes(Thread*)+0x5f
V [libjvm.dylib+0x97f525] MetaspaceShared::preload_and_dump(Thread*)+0x1bb
V [libjvm.dylib+0xb8196c] Threads::create_vm(JavaVMInitArgs*, bool*)+0x972
V [libjvm.dylib+0x6ef71c] JNI_CreateJavaVM+0xac
C [libjli.dylib+0x52bb] JavaMain+0x10b
C [libjli.dylib+0x7e32] ThreadJavaMain+0x9
C [libsystem_pthread.dylib+0x68fc] _pthread_start+0xe0
C [libsystem_pthread.dylib+0x2443] thread_start+0xf

-----Original Message-----
From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On Behalf Of Zhengyu Gu
Sent: Samstag, 6. November 2021 23:29
To: jdk-updates-dev at openjdk.java.net
Subject: [jdk11u-dev] Integrated: 8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror"

On Thu, 4 Nov 2021 16:32:16 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

Backport for parity with Oracle 11.0.15.

The original patch applies cleanly, but some of CDS tests fail due to following new assertion in `ClassLoaderData::loaded_classes_do()`

` // To call this, one must have the MultiArray_lock held, but the _klasses list still has lock free reads.
assert_locked_or_safepoint(MultiArray_lock);
`
The solution is to take `MultiArray_lock` in `MetaspaceShared::link_and_cleanup_shared_classes()` method

Test:
- [x] tier1

This pull request has now been integrated.

Changeset: 3a35158
Author: Zhengyu Gu <zgu at openjdk.org>
URL: https://git.openjdk.java.net/jdk11u-dev/commit/3a3515807cd76b571aa9cf5f19203bca7be7f970
Stats: 9 lines in 3 files changed: 9 ins; 0 del; 0 mod

8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror"

Reviewed-by: phh
Backport-of: 172aed1a2d75756b140cb723133ac5fb67f7745e

-------------

PR: https://git.openjdk.java.net/jdk11u-dev/pull/584

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 10, 2021

Mailing list message from David Holmes on jdk-updates-dev:

On 9/11/2021 10:50 pm, Lindenmaier, Goetz wrote:

Hi Zhengyu,

Since this change has been pushed, we see four test crashing in our CI,
see below. Do you have an idea what is wrong with these?

From JBS:

"The original patch applies cleanly, but needs to take additional
MultiArray_lock on CDS path to get it work correctly. "

but that additional change is broken as you can't hold the lock on that
path due to the linking that can occur which requires a call up to Java
which can't be done when holding the mutex. I don't know why Zhengyu
thought that was neccessary.

David
-----

@zhengyu123
Copy link
Contributor Author

@zhengyu123 zhengyu123 commented Dec 6, 2021

Mailing list message from David Holmes on jdk-updates-dev:

On 9/11/2021 10:50 pm, Lindenmaier, Goetz wrote:

Hi Zhengyu,
Since this change has been pushed, we see four test crashing in our CI,
see below. Do you have an idea what is wrong with these?

From JBS:

"The original patch applies cleanly, but needs to take additional MultiArray_lock on CDS path to get it work correctly. "

but that additional change is broken as you can't hold the lock on that path due to the linking that can occur which requires a call up to Java which can't be done when holding the mutex. I don't know why Zhengyu thought that was neccessary.

David -----

Hi David,

Sorry, I did not see your comment until now.

Adding MultiArray_lock on CDS path, as stated in comment, was to workaround the new assertion in ClassLoaderData::loaded_classes_do() in following code:

void MetaspaceShared::link_and_cleanup_shared_classes(TRAPS) {
// We need to iterate because verification may cause additional classes
// to be loaded.
LinkSharedClassesClosure link_closure(THREAD);
do {
link_closure.reset();
ClassLoaderDataGraph::loaded_classes_do(&link_closure);
guarantee(!HAS_PENDING_EXCEPTION, "exception in link_class");
} while (link_closure.made_progress());

I revised patch to drop the new assertion in ClassLoaderData::loaded_classes_do() https://github.com/openjdk/jdk11u-dev/pull/632/files

Thanks,

-Zhengyu

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