From 63ce88b5fbc8e2b9be01a135156885000bc5c48d Mon Sep 17 00:00:00 2001 From: David Holmes Date: Mon, 27 Mar 2023 22:05:23 +0000 Subject: [PATCH] 8304147: JVM crash during shutdown when dumping dynamic archive Reviewed-by: ccheung, matsaave, coleenp --- src/hotspot/share/cds/dynamicArchive.cpp | 49 +++++++------ src/hotspot/share/cds/dynamicArchive.hpp | 4 +- src/hotspot/share/prims/jvm.cpp | 6 -- src/hotspot/share/runtime/java.cpp | 23 +++--- src/hotspot/share/runtime/javaThread.cpp | 13 +--- .../appcds/dynamicArchive/ExitRaceTest.java | 59 +++++++++++++++ .../dynamicArchive/test-classes/ExitRace.java | 71 +++++++++++++++++++ 7 files changed, 169 insertions(+), 56 deletions(-) create mode 100644 test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ExitRaceTest.java create mode 100644 test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/ExitRace.java diff --git a/src/hotspot/share/cds/dynamicArchive.cpp b/src/hotspot/share/cds/dynamicArchive.cpp index 8047544fe7e..707f0d5e700 100644 --- a/src/hotspot/share/cds/dynamicArchive.cpp +++ b/src/hotspot/share/cds/dynamicArchive.cpp @@ -377,18 +377,36 @@ void DynamicArchive::check_for_dynamic_dump() { } } -void DynamicArchive::prepare_for_dump_at_exit() { - EXCEPTION_MARK; - ResourceMark rm(THREAD); - MetaspaceShared::link_shared_classes(false/*not from jcmd*/, THREAD); - if (HAS_PENDING_EXCEPTION) { - log_error(cds)("Dynamic dump has failed"); - log_error(cds)("%s: %s", PENDING_EXCEPTION->klass()->external_name(), - java_lang_String::as_utf8_string(java_lang_Throwable::message(PENDING_EXCEPTION))); - // We cannot continue to dump the archive anymore. - DynamicDumpSharedSpaces = false; - CLEAR_PENDING_EXCEPTION; +void DynamicArchive::dump_at_exit(JavaThread* current, const char* archive_name) { + ExceptionMark em(current); + ResourceMark rm(current); + HandleMark hm(current); + + if (!DynamicDumpSharedSpaces || archive_name == nullptr) { + return; } + + log_info(cds, dynamic)("Preparing for dynamic dump at exit in thread %s", current->name()); + + JavaThread* THREAD = current; // For TRAPS processing related to link_shared_classes + MetaspaceShared::link_shared_classes(false/*not from jcmd*/, THREAD); + if (!HAS_PENDING_EXCEPTION) { + // copy shared path table to saved. + FileMapInfo::clone_shared_path_table(current); + if (!HAS_PENDING_EXCEPTION) { + VM_PopulateDynamicDumpSharedSpace op(archive_name); + VMThread::execute(&op); + return; + } + } + + // One of the prepatory steps failed + oop ex = current->pending_exception(); + log_error(cds)("Dynamic dump has failed"); + log_error(cds)("%s: %s", ex->klass()->external_name(), + java_lang_String::as_utf8_string(java_lang_Throwable::message(ex))); + CLEAR_PENDING_EXCEPTION; + DynamicDumpSharedSpaces = false; // Just for good measure } // This is called by "jcmd VM.cds dynamic_dump" @@ -397,21 +415,12 @@ void DynamicArchive::dump_for_jcmd(const char* archive_name, TRAPS) { assert(ArchiveClassesAtExit == nullptr, "already checked in arguments.cpp"); assert(DynamicDumpSharedSpaces, "already checked by check_for_dynamic_dump() during VM startup"); MetaspaceShared::link_shared_classes(true/*from jcmd*/, CHECK); - dump(archive_name, THREAD); -} - -void DynamicArchive::dump(const char* archive_name, TRAPS) { // copy shared path table to saved. FileMapInfo::clone_shared_path_table(CHECK); - VM_PopulateDynamicDumpSharedSpace op(archive_name); VMThread::execute(&op); } -bool DynamicArchive::should_dump_at_vm_exit() { - return DynamicDumpSharedSpaces && (ArchiveClassesAtExit != nullptr); -} - bool DynamicArchive::validate(FileMapInfo* dynamic_info) { assert(!dynamic_info->is_static(), "must be"); // Check if the recorded base archive matches with the current one diff --git a/src/hotspot/share/cds/dynamicArchive.hpp b/src/hotspot/share/cds/dynamicArchive.hpp index a56b91af69a..2cd03b5b4e7 100644 --- a/src/hotspot/share/cds/dynamicArchive.hpp +++ b/src/hotspot/share/cds/dynamicArchive.hpp @@ -59,10 +59,8 @@ public: class DynamicArchive : AllStatic { public: static void check_for_dynamic_dump(); - static bool should_dump_at_vm_exit(); - static void prepare_for_dump_at_exit(); static void dump_for_jcmd(const char* archive_name, TRAPS); - static void dump(const char* archive_name, TRAPS); + static void dump_at_exit(JavaThread* current, const char* archive_name); static bool is_mapped() { return FileMapInfo::dynamic_info() != nullptr; } static bool validate(FileMapInfo* dynamic_info); }; diff --git a/src/hotspot/share/prims/jvm.cpp b/src/hotspot/share/prims/jvm.cpp index 25a1835cd6c..8bdba0ad605 100644 --- a/src/hotspot/share/prims/jvm.cpp +++ b/src/hotspot/share/prims/jvm.cpp @@ -426,12 +426,6 @@ JVM_END extern volatile jint vm_created; JVM_ENTRY_NO_ENV(void, JVM_BeforeHalt()) -#if INCLUDE_CDS - // Link all classes for dynamic CDS dumping before vm exit. - if (DynamicArchive::should_dump_at_vm_exit()) { - DynamicArchive::prepare_for_dump_at_exit(); - } -#endif EventShutdown event; if (event.should_commit()) { event.set_reason("Shutdown requested from Java"); diff --git a/src/hotspot/share/runtime/java.cpp b/src/hotspot/share/runtime/java.cpp index 91353fc04fa..3b2cd631f7e 100644 --- a/src/hotspot/share/runtime/java.cpp +++ b/src/hotspot/share/runtime/java.cpp @@ -439,6 +439,14 @@ void before_exit(JavaThread* thread, bool halt) { } #endif +#if INCLUDE_CDS + // Dynamic CDS dumping must happen whilst we can still reliably + // run Java code. + DynamicArchive::dump_at_exit(thread, ArchiveClassesAtExit); + assert(!thread->has_pending_exception(), "must be"); +#endif + + // Actual shutdown logic begins here. #if INCLUDE_JVMCI @@ -514,21 +522,6 @@ void before_exit(JavaThread* thread, bool halt) { // Note: we don't wait until it actually dies. os::terminate_signal_thread(); -#if INCLUDE_CDS - if (DynamicArchive::should_dump_at_vm_exit()) { - assert(ArchiveClassesAtExit != nullptr, "Must be already set"); - ExceptionMark em(thread); - DynamicArchive::dump(ArchiveClassesAtExit, thread); - if (thread->has_pending_exception()) { - ResourceMark rm(thread); - oop pending_exception = thread->pending_exception(); - log_error(cds)("ArchiveClassesAtExit has failed %s: %s", pending_exception->klass()->external_name(), - java_lang_String::as_utf8_string(java_lang_Throwable::message(pending_exception))); - thread->clear_pending_exception(); - } - } -#endif - print_statistics(); Universe::heap()->print_tracing_info(); diff --git a/src/hotspot/share/runtime/javaThread.cpp b/src/hotspot/share/runtime/javaThread.cpp index f1c28cbf846..36c7cc295b9 100644 --- a/src/hotspot/share/runtime/javaThread.cpp +++ b/src/hotspot/share/runtime/javaThread.cpp @@ -2023,22 +2023,11 @@ bool JavaThread::sleep(jlong millis) { void JavaThread::invoke_shutdown_hooks() { HandleMark hm(this); - // We could get here with a pending exception, if so clear it now or - // it will cause MetaspaceShared::link_shared_classes to - // fail for dynamic dump. + // We could get here with a pending exception, if so clear it now. if (this->has_pending_exception()) { this->clear_pending_exception(); } -#if INCLUDE_CDS - // Link all classes for dynamic CDS dumping before vm exit. - // Same operation is being done in JVM_BeforeHalt for handling the - // case where the application calls System.exit(). - if (DynamicArchive::should_dump_at_vm_exit()) { - DynamicArchive::prepare_for_dump_at_exit(); - } -#endif - EXCEPTION_MARK; Klass* shutdown_klass = SystemDictionary::resolve_or_null(vmSymbols::java_lang_Shutdown(), diff --git a/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ExitRaceTest.java b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ExitRaceTest.java new file mode 100644 index 00000000000..1370fce03cb --- /dev/null +++ b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ExitRaceTest.java @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +/* + * @test + * @bug 8304147 + * @summary Test the exit race for dynamic dumping at exit + * @requires vm.cds + * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds /test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes + * @build ExitRace jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar exitRace.jar ExitRace ExitRace$1 ExitRace$1$1 + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. ExitRaceTest + */ + +import jdk.test.lib.helpers.ClassFileInstaller; + +public class ExitRaceTest extends DynamicArchiveTestBase { + + public static void main(String[] args) throws Exception { + runTest(ExitRaceTest::test); + } + + static void test() throws Exception { + String topArchiveName = getNewArchiveName(); + String appJar = ClassFileInstaller.getJarPath("exitRace.jar"); + String mainClass = "ExitRace"; + + dump(topArchiveName, + "-Xlog:cds+dynamic=debug", + "-cp", appJar, mainClass) + .assertNormalExit(output -> { + output.shouldHaveExitValue(0) + .shouldContain("Preparing for dynamic dump") + .reportDiagnosticSummary(); + }); + } +} diff --git a/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/ExitRace.java b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/ExitRace.java new file mode 100644 index 00000000000..9b9a940c0b3 --- /dev/null +++ b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/ExitRace.java @@ -0,0 +1,71 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +// Try to trigger concurrent dynamic-dump-at-exit processing by creating a race +// between normal VM termination by the last non-daemon thread exiting, and a +// call to Runtime.halt(). + +public class ExitRace { + + static volatile int terminationPhase = 0; + + public static void main(String [] args) { + + // Need to spawn a new non-daemon thread so the main thread will + // have time to become the DestroyJavaVM thread. + Thread runner = new Thread("Runner") { + public void run() { + // This thread will be the one to trigger normal VM termination + // when it exits. We first create a daemon thread to call + // Runtime.halt() and then wait for it to tell us to exit. + + Thread daemon = new Thread("Daemon") { + public void run() { + // Let main thread go + terminationPhase = 1; + // Spin until main thread is active again + while (terminationPhase == 1) + ; + Runtime.getRuntime().halt(0); // Normal exit code + } + }; + daemon.setDaemon(true); + daemon.start(); + + // Wait until daemon is ready + while (terminationPhase == 0) { + try { + Thread.sleep(10); + } catch (InterruptedException cantHappen) { + } + } + + // Release daemon thread + terminationPhase++; + // Normal exit + } + }; + runner.start(); + } +}