Fix according to review comment and add more tests

src/hotspot/share/memory/metaspaceShared.cpp

@@ -47,7 +47,6 @@
 #include "memory/archiveBuilder.hpp"
 #include "memory/cppVtables.hpp"
 #include "memory/dumpAllocStats.hpp"
-#include "memory/dynamicArchive.hpp"
 #include "memory/filemap.hpp"
 #include "memory/heapShared.hpp"
 #include "memory/metaspace.hpp"

src/hotspot/share/oops/instanceKlass.cpp

@@ -4257,54 +4257,50 @@ bool InstanceKlass::is_shareable() const {
 
 void InstanceKlass::log_to_classlist(const ClassFileStream* stream) const {
 #if INCLUDE_CDS
+  ResourceMark rm;
   if (ClassListWriter::is_enabled()) {
     if (!ClassLoader::has_jrt_entry()) {
        warning("DumpLoadedClassList and CDS are not supported in exploded build");
        DumpLoadedClassList = NULL;
        return;
     }
-    ClassLoaderData* loader_data = class_loader_data();
-    if (!SystemDictionaryShared::is_sharing_possible(loader_data)) {
-      return;
-    }
-    bool skip = false;
-    if (is_shared()) {
-      assert(stream == NULL, "shared class with stream");
-      if (is_hidden()) {
-        // Don't include archived lambda proxy class in the classlist.
-        assert(!is_non_strong_hidden(), "unexpected non-strong hidden class");
-        return;
-      }
-    } else {
-      assert(stream != NULL, "non-shared class without stream");
-      // skip hidden class and unsafe anonymous class.
-      if ( is_hidden() || unsafe_anonymous_host() != NULL) {
-        return;
-      }
-      oop class_loader = loader_data->class_loader();
-      if (class_loader == NULL || SystemDictionary::is_platform_class_loader(class_loader)) {
-        // For the boot and platform class loaders, skip classes that are not found in the
-        // java runtime image, such as those found in the --patch-module entries.
-        // These classes can't be loaded from the archive during runtime.
-        if (!stream->from_boot_loader_modules_image() && strncmp(stream->source(), "jrt:", 4) != 0) {
-          skip = true;
+    if (is_shareable()) {
+      bool skip = false;
+      if (is_shared()) {
+        if (stream == nullptr) {
+          tty->print_cr("%s: shared class with stream?", name()->as_C_string());
+          return; // stream is nullptr will cause below operation fail.
         }
+      } else {
+        if (stream != nullptr) {
+          oop class_loader = class_loader_data()->class_loader();
+          if (class_loader == NULL || SystemDictionary::is_platform_class_loader(class_loader)) {
+            // For the boot and platform class loaders, skip classes that are not found in the
+            // java runtime image, such as those found in the --patch-module entries.
+            // These classes can't be loaded from the archive during runtime.
+            if (!stream->from_boot_loader_modules_image() && strncmp(stream->source(), "jrt:", 4) != 0) {
+              skip = true;
+            }
 
-        if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {
-          // .. but don't skip the boot classes that are loaded from -Xbootclasspath/a
-          // as they can be loaded from the archive during runtime.
-          skip = false;
+            if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {
+              // .. but don't skip the boot classes that are loaded from -Xbootclasspath/a
+              // as they can be loaded from the archive during runtime.
+              skip = false;
+            }
+          }
+        } else {
+          warning("non-shared class without stream, skipped");
+          return;
         }
       }
-    }
-    ResourceMark rm;
-    if (skip) {
-      tty->print_cr("skip writing class %s from source %s to classlist file",
-                    name()->as_C_string(), stream->source());
-    } else {
-      ClassListWriter w;
-      w.stream()->print_cr("%s", name()->as_C_string());
-      w.stream()->flush();
+      if (skip) {
+        tty->print_cr("skip writing class %s from source %s to classlist file",
+                      name()->as_C_string(), stream->source());
+      } else {
+        ClassListWriter w;
+        w.stream()->print_cr("%s", name()->as_C_string());
+        w.stream()->flush();
+      }
     }
   }
 #endif // INCLUDE_CDS

src/hotspot/share/prims/jvm.cpp

@@ -3763,15 +3763,15 @@ JVM_END
 
 JVM_ENTRY(void, JVM_DumpDynamicArchive(JNIEnv *env, jstring archiveName))
 #if INCLUDE_CDS
-  assert(UseSharedSpaces && RecordDynamicDumpInfo, "Sanity check");
+  assert(UseSharedSpaces && RecordDynamicDumpInfo, "already checked in arguments.cpp?");
   if (DynamicArchive::has_been_dumped_once()) {
     THROW_MSG(vmSymbols::java_lang_RuntimeException(),
         "Dynamic dump has been done, and should only be done once");
   } else {
     // prevent multiple dumps.
     DynamicArchive::set_has_been_dumped_once();
   }
-  assert(ArchiveClassesAtExit == nullptr, "Sanity check");
+  assert(ArchiveClassesAtExit == nullptr, "already checked in arguments.cpp?");
   Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
   char* archive_name  = java_lang_String::as_utf8_string(file_handle());
   ArchiveClassesAtExit = archive_name;

src/hotspot/share/services/diagnosticCommand.cpp

@@ -1126,19 +1126,11 @@ void DumpSharedArchiveDCmd::execute(DCmdSource source, TRAPS) {
   JavaCallArguments args;
   args.push_int(is_static);
   args.push_oop(fileh);
-  output()->print_cr("Call CDS.dumpSharedArchive(%s, %s)", scmd, (file == NULL ? "null" : file));
   JavaCalls::call_static(&result,
                          cds_klass,
                          vmSymbols::dumpSharedArchive(),
                          vmSymbols::dumpSharedArchive_signature(),
                          &args, THREAD);
-  // Upon exception, show stack trace.
-  if (HAS_PENDING_EXCEPTION) {
-    Handle throwable(THREAD, PENDING_EXCEPTION);
-    CLEAR_PENDING_EXCEPTION;
-    java_lang_Throwable::print_stack_trace(throwable, output());
-    output()->cr();
-  }
 }
 
 int DumpSharedArchiveDCmd::num_arguments() {

src/java.base/share/classes/jdk/internal/misc/CDS.java

@@ -28,6 +28,7 @@
 import java.io.File;
 import java.io.InputStream;
 import java.io.IOException;
+import java.io.PrintStream;
 import java.util.Arrays;
 import java.util.ArrayList;
 import java.util.Map;
@@ -207,13 +208,15 @@ private static Object[] generateLambdaFormHolderClasses(String[] lines) {
     private static native void dumpClassList(String listFileName);
     private static native void dumpDynamicArchive(String archiveFileName);
 
-    private static void outputStdStream(InputStream stream) {
+    private static void outputStdStream(InputStream stream, String fileName) {
         String line;
         InputStreamReader isr = new InputStreamReader(stream);
         BufferedReader rdr = new BufferedReader(isr);
+
         try {
+            PrintStream prt = new PrintStream(fileName);
             while((line = rdr.readLine()) != null) {
-                System.out.println(line);
+                prt.println(line);
             }
         } catch (IOException e) {
             throw new RuntimeException("IOExeption happens during drain stream " + e.getMessage());
@@ -246,9 +249,9 @@ private static boolean containsExcludedFlags(String testStr) {
     * @param fileName user input archive name, can be null.
     */
     private static void dumpSharedArchive(boolean isStatic, String fileName) throws Exception {
+        String currentPid = String.valueOf(ProcessHandle.current().pid());
         String archiveFile =  fileName != null ? fileName :
-            "java_pid" + ProcessHandle.current().pid() + (isStatic ? "_static.jsa" : "_dynamic.jsa");
-        System.out.println((isStatic ? "Static" : " Dynamic") + " dump to file " + archiveFile);
+            "java_pid" + currentPid + (isStatic ? "_static.jsa" : "_dynamic.jsa");
 
         // delete if archive file aready exists
         File fileArchive = new File(archiveFile);
@@ -290,26 +293,40 @@ private static void dumpSharedArchive(boolean isStatic, String fileName) throws
             Process proc = Runtime.getRuntime().exec(cmds.toArray(new String[0]),
                                new String[] {"EnvP=null"});
 
-            // Drain stdout in a separate thread.
+            // Drain stdout to a file in a separate thread.
+            String stdOutFile = "java_pid" + proc.pid() + "_stdout.txt";
             new Thread( ()-> {
-                    System.out.println("Dumping process " + proc.pid() + " Stdout: ");
-                    outputStdStream(proc.getInputStream());
+                    outputStdStream(proc.getInputStream(), stdOutFile);
                 }).start();
 
-            // Drain stderr in a separate thread.
+            // Drain stderr to a file in a separate thread.
+            String stdErrFile = "java_pid" + proc.pid() + "_stderr.txt";
             new Thread( ()-> {
-                    System.out.println("Dumping process " + proc.pid() + " Stdout: ");
-                    outputStdStream(proc.getErrorStream());
+                    outputStdStream(proc.getErrorStream(), stdErrFile);
                 }).start();
 
             proc.waitFor();
+            // done, delete classlist file.
+            if (fileList.exists()) {
+                fileList.delete();
+            }
+            // Check if archive has been successfully dumped. We won't reach here if exception happens.
+            // Throw exception if file is not created.
+            if (!fileArchive.exists()) {
+                throw new RuntimeException("Archive file " + archiveFile +
+                                           " is not created, please check stdout file " +
+                                            stdOutFile + " or stderr file " +
+                                            stdErrFile + " for more detail");
+            }
         } else {
             dumpDynamicArchive(archiveFile);
+            if (!fileArchive.exists()) {
+                throw new RuntimeException("Archive file " + archiveFile +
+                                           " is not created, please check process " +
+                                           currentPid + " output for more detail");
+            }
         }
-        // Check if archive has been successfully dumped. We won't reach here if exception happens.
-        // Throw exception if file is not created.
-        if (!fileArchive.exists()) {
-            throw new RuntimeException("Archive file " + archiveFile + " is not created");
-        }
+        // Everyting goes well, print out the file name.
+        System.out.println((isStatic ? "Static" : " Dynamic") + " dump to file " + archiveFile);
     }
 }

test/hotspot/jtreg/TEST.groups

@@ -324,6 +324,7 @@ hotspot_appcds_dynamic = \
  -runtime/cds/appcds/javaldr/GCSharedStringsDuringDump.java \
  -runtime/cds/appcds/javaldr/HumongousDuringDump.java \
  -runtime/cds/appcds/javaldr/LockDuringDump.java \
+ -runtime/cds/appcds/jcmd/JCmdTest.java \
  -runtime/cds/appcds/methodHandles \
  -runtime/cds/appcds/sharedStrings \
  -runtime/cds/appcds/ArchiveRelocationTest.java \

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java

@@ -192,11 +192,11 @@ private static void test() throws Exception {
         }
         app.stopApp();
 
-        // 3. Test static dump with flags which no dump will happen.
+        // 3. Test static dump with flags with which dumping should fail
         //    This test will result classes.jsa in default server dir if -XX:SharedArchiveFile= not set.
-        print2ln("3. Test static dump with flags which will no dump will happen.");
+        print2ln("3. Test static dump with flags with which dumping should fail.");
         for (String flag : noDumpFlags) {
-            app  = createLingeredApp("-cp", jarFile, flag, "-XX:SharedArchiveFile=tmp.jsa");
+            app = createLingeredApp("-cp", jarFile, flag, "-XX:SharedArchiveFile=tmp.jsa");
             // Following should not be executed.
             if (app != null && app.getProcess().isAlive()) {
                 pid = app.getPid();
@@ -210,7 +210,7 @@ private static void test() throws Exception {
         // 4. Test static dump with flags which will be filtered before dumping.
         print2ln("4. Test static dump with flags which will be filtered before dumping.");
         for (String flag : excludeFlags) {
-            app  = createLingeredApp("-cp", jarFile, flag);
+            app = createLingeredApp("-cp", jarFile, flag);
             pid = app.getPid();
             test(SUBCMD_STATIC_DUMP, null, pid, EXPECT_PASS);
             app.stopApp();
@@ -219,37 +219,58 @@ private static void test() throws Exception {
 
         // 5. Test static with -Xshare:off will be OK to dump.
         print2ln("5. Test static with -Xshare:off will be OK to dump.");
-        app  = createLingeredApp("-Xshare:off", "-cp", jarFile);
+        app = createLingeredApp("-Xshare:off", "-cp", jarFile);
         pid = app.getPid();
         test(SUBCMD_STATIC_DUMP, null, pid, EXPECT_PASS);
         app.stopApp();
 
-        // 6. Test dynamic dump with -XX:+RecordDynamicDumpInfo.
-        print2ln("6. Test dynamic dump with -XX:+RecordDynamicDumpInfo.");
-        app  = createLingeredApp("-cp", jarFile, "-XX:+RecordDynamicDumpInfo");
+        // 6. Test static with --limit-modules java.base.
+        print2ln("6. Test static with --limit-modules java.base.");
+        app = createLingeredApp("--limit-modules", "java.base", "-cp", jarFile);
+        pid = app.getPid();
+        test(SUBCMD_STATIC_DUMP, null, pid, EXPECT_FAIL);
+
+        // 7. Test dynamic dump with -XX:+RecordDynamicDumpInfo.
+        print2ln("7. Test dynamic dump with -XX:+RecordDynamicDumpInfo.");
+        app = createLingeredApp("-cp", jarFile, "-XX:+RecordDynamicDumpInfo");
         pid = app.getPid();
         test(SUBCMD_DYNAMIC_DUMP, DYNAMIC_DUMP_FILE + "01", pid, EXPECT_PASS);
 
-        // 7. Test dynamic dump twice to same process.
-        print2ln("7. Test dynamic dump second time to the same process.");
+        // 8. Test dynamic dump twice to same process.
+        print2ln("8. Test dynamic dump second time to the same process.");
         test(SUBCMD_DYNAMIC_DUMP, DYNAMIC_DUMP_FILE + "02", pid, EXPECT_FAIL);
         app.stopApp();
 
-        // 8. Test dynamic dump with -XX:-RecordDynamicDumpInfo.
-        print2ln("8. Test dynamic dump with -XX:-RecordDynamicDumpInfo.");
-        app  = createLingeredApp("-cp", jarFile);
+        // 9. Test dynamic dump with -XX:-RecordDynamicDumpInfo.
+        print2ln("9. Test dynamic dump with -XX:-RecordDynamicDumpInfo.");
+        app = createLingeredApp("-cp", jarFile);
         pid = app.getPid();
         test(SUBCMD_DYNAMIC_DUMP, DYNAMIC_DUMP_FILE + "01", pid, EXPECT_FAIL);
         app.stopApp();
 
-        // 9. Test dynamic dump with default archive name (null).
-        print2ln("9. Test dynamic dump with default archive name (null).");
+        // 10. Test dynamic dump with default archive name (null).
+        print2ln("10. Test dynamic dump with default archive name (null).");
         app = createLingeredApp("-cp", jarFile, "-XX:+RecordDynamicDumpInfo");
         pid = app.getPid();
         test(SUBCMD_DYNAMIC_DUMP, null, pid, EXPECT_PASS);
+        app.stopApp();
+
+        // 11. Test dynamic dump with flags -XX:+RecordDynamicDumpInfo -XX:-DynamicDumpSharedSpaces.
+        print2ln("11. Test dynamic dump with flags -XX:+RecordDynamicDumpInfo -XX:-DynamicDumpSharedSpaces.");
+        app = createLingeredApp("-cp", jarFile, "-XX:+RecordDynamicDumpInfo", "-XX:-DynamicDumpSharedSpaces");
+        pid = app.getPid();
+        test(SUBCMD_DYNAMIC_DUMP, null, pid, EXPECT_PASS);
+        app.stopApp();
+
+        // 12. Test dynamic dump with flags -XX:-DynamicDumpSharedSpaces -XX:+RecordDynamicDumpInfo.
+        print2ln("12. Test dynamic dump with flags -XX:-DynamicDumpSharedSpaces -XX:+RecordDynamicDumpInfo.");
+        app = createLingeredApp("-cp", jarFile, "-XX:-DynamicDumpSharedSpaces", "-XX:+RecordDynamicDumpInfo");
+        pid = app.getPid();
+        test(SUBCMD_DYNAMIC_DUMP, null, pid, EXPECT_PASS);
+        app.stopApp();
 
-        // 10. Test dynamic dump with -XX:ArchiveClassAtExit will fail.
-        print2ln("10. Test dynamic dump with -XX:ArchiveClassAtExit will fail.");
+        // 13. Test dynamic dump with -XX:ArchiveClassAtExit will fail.
+        print2ln("13. Test dynamic dump with -XX:ArchiveClassAtExit will fail.");
         app = createLingeredApp("-cp", jarFile,
                                 "-Xshare:auto",
                                 "-XX:+RecordDynamicDumpInfo",