8334492: DiagnosticCommands (jcmd) should accept %p in output filenam…

…es and substitute PID

Reviewed-by: kevinw, stuefe, lmesnik

src/hotspot/share/code/codeCache.cpp

@@ -1790,15 +1790,17 @@ void CodeCache::log_state(outputStream* st) {
 #ifdef LINUX
 void CodeCache::write_perf_map(const char* filename, outputStream* st) {
   MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
-
-  // Perf expects to find the map file at /tmp/perf-<pid>.map
-  // if the file name is not specified.
-  char fname[32];
+  char fname[JVM_MAXPATHLEN];
   if (filename == nullptr) {
-    jio_snprintf(fname, sizeof(fname), "/tmp/perf-%d.map", os::current_process_id());
+    // Invocation outside of jcmd requires pid substitution.
+    if (!Arguments::copy_expand_pid(DEFAULT_PERFMAP_FILENAME,
+                                    strlen(DEFAULT_PERFMAP_FILENAME),
+                                    fname, JVM_MAXPATHLEN)) {
+      st->print_cr("Warning: Not writing perf map as pid substitution failed.");
+      return;
+    }
     filename = fname;
   }
-
   fileStream fs(filename, "w");
   if (!fs.is_open()) {
     st->print_cr("Warning: Failed to create %s for perf map", filename);

src/hotspot/share/code/codeCache.hpp

@@ -80,6 +80,10 @@ class ShenandoahParallelCodeHeapIterator;
 class NativePostCallNop;
 class DeoptimizationScope;
 
+#ifdef LINUX
+#define DEFAULT_PERFMAP_FILENAME "/tmp/perf-%p.map"
+#endif
+
 class CodeCache : AllStatic {
   friend class VMStructs;
   friend class JVMCIVMStructs;

src/hotspot/share/prims/wbtestmethods/parserTests.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2024, 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
@@ -127,6 +127,14 @@ static void fill_in_parser(DCmdParser* parser, oop argument)
      } else {
       parser->add_dcmd_option(argument);
      }
+   } else if (strcmp(type, "FILE") == 0) {
+      DCmdArgument<char*>* argument =
+          new DCmdArgument<char*>(name, desc, "FILE", mandatory);
+      if (isarg) {
+        parser->add_dcmd_argument(argument);
+      } else {
+        parser->add_dcmd_option(argument);
+      }
    }
 }
 

src/hotspot/share/services/diagnosticArgument.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2024, 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
@@ -189,9 +189,18 @@ template <> void DCmdArgument<char*>::parse_value(const char* str,
     destroy_value();
   } else {
     // Use realloc as we may have a default set.
-    _value = REALLOC_C_HEAP_ARRAY(char, _value, len + 1, mtInternal);
-    int n = os::snprintf(_value, len + 1, "%.*s", (int)len, str);
-    assert((size_t)n <= len, "Unexpected number of characters in string");
+    if (strcmp(type(), "FILE") == 0) {
+      _value = REALLOC_C_HEAP_ARRAY(char, _value, JVM_MAXPATHLEN, mtInternal);
+      if (!Arguments::copy_expand_pid(str, len, _value, JVM_MAXPATHLEN)) {
+        stringStream error_msg;
+        error_msg.print("File path invalid or too long: %s", str);
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), error_msg.base());
+      }
+    } else {
+      _value = REALLOC_C_HEAP_ARRAY(char, _value, len + 1, mtInternal);
+      int n = os::snprintf(_value, len + 1, "%.*s", (int)len, str);
+      assert((size_t)n <= len, "Unexpected number of characters in string");
+    }
   }
 }
 

src/hotspot/share/services/diagnosticArgument.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, 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
@@ -27,6 +27,7 @@
 
 #include "classfile/vmSymbols.hpp"
 #include "memory/allocation.hpp"
+#include "runtime/arguments.hpp"
 #include "runtime/javaThread.hpp"
 #include "runtime/os.hpp"
 #include "utilities/exceptions.hpp"

src/hotspot/share/services/diagnosticCommand.cpp

@@ -473,7 +473,7 @@ void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
 #if INCLUDE_SERVICES // Heap dumping/inspection supported
 HeapDumpDCmd::HeapDumpDCmd(outputStream* output, bool heap) :
                            DCmdWithParser(output, heap),
-  _filename("filename","Name of the dump file", "STRING",true),
+  _filename("filename","Name of the dump file", "FILE",true),
   _all("-all", "Dump all objects, including unreachable objects",
        "BOOLEAN", false, "false"),
   _gzip("-gz", "If specified, the heap dump is written in gzipped format "
@@ -852,20 +852,15 @@ void CodeCacheDCmd::execute(DCmdSource source, TRAPS) {
 }
 
 #ifdef LINUX
-#define DEFAULT_PERFMAP_FILENAME "/tmp/perf-<pid>.map"
-
 PerfMapDCmd::PerfMapDCmd(outputStream* output, bool heap) :
              DCmdWithParser(output, heap),
-  _filename("filename", "Name of the map file", "STRING", false, DEFAULT_PERFMAP_FILENAME)
+  _filename("filename", "Name of the map file", "FILE", false, DEFAULT_PERFMAP_FILENAME)
 {
   _dcmdparser.add_dcmd_argument(&_filename);
 }
 
 void PerfMapDCmd::execute(DCmdSource source, TRAPS) {
-  // The check for _filename.is_set() is because we don't want to use
-  // DEFAULT_PERFMAP_FILENAME, since it is meant as a description
-  // of the default, not the actual default.
-  CodeCache::write_perf_map(_filename.is_set() ? _filename.value() : nullptr, output());
+  CodeCache::write_perf_map(_filename.value(), output());
 }
 #endif // LINUX
 
@@ -997,12 +992,12 @@ void ClassesDCmd::execute(DCmdSource source, TRAPS) {
 }
 
 #if INCLUDE_CDS
-#define DEFAULT_CDS_ARCHIVE_FILENAME "java_pid<pid>_<subcmd>.jsa"
+#define DEFAULT_CDS_ARCHIVE_FILENAME "java_pid%p_<subcmd>.jsa"
 
 DumpSharedArchiveDCmd::DumpSharedArchiveDCmd(outputStream* output, bool heap) :
                                      DCmdWithParser(output, heap),
   _suboption("subcmd", "static_dump | dynamic_dump", "STRING", true),
-  _filename("filename", "Name of shared archive to be dumped", "STRING", false,
+  _filename("filename", "Name of shared archive to be dumped", "FILE", false,
             DEFAULT_CDS_ARCHIVE_FILENAME)
 {
   _dcmdparser.add_dcmd_argument(&_suboption);
@@ -1040,7 +1035,7 @@ void DumpSharedArchiveDCmd::execute(DCmdSource source, TRAPS) {
   // call CDS.dumpSharedArchive
   Handle fileh;
   if (file != nullptr) {
-    fileh =  java_lang_String::create_from_str(_filename.value(), CHECK);
+    fileh = java_lang_String::create_from_str(file, CHECK);
   }
   Symbol* cds_name  = vmSymbols::jdk_internal_misc_CDS();
   Klass*  cds_klass = SystemDictionary::resolve_or_fail(cds_name, true /*throw error*/,  CHECK);
@@ -1105,7 +1100,7 @@ ThreadDumpToFileDCmd::ThreadDumpToFileDCmd(outputStream* output, bool heap) :
                                            DCmdWithParser(output, heap),
   _overwrite("-overwrite", "May overwrite existing file", "BOOLEAN", false, "false"),
   _format("-format", "Output format (\"plain\" or \"json\")", "STRING", false, "plain"),
-  _filepath("filepath", "The file path to the output file", "STRING", true) {
+  _filepath("filepath", "The file path to the output file", "FILE", true) {
   _dcmdparser.add_dcmd_option(&_overwrite);
   _dcmdparser.add_dcmd_option(&_format);
   _dcmdparser.add_dcmd_argument(&_filepath);
@@ -1186,23 +1181,16 @@ void SystemMapDCmd::execute(DCmdSource source, TRAPS) {
   MemMapPrinter::print_all_mappings(output());
 }
 
-static constexpr char default_filename[] = "vm_memory_map_<pid>.txt";
+static constexpr char default_filename[] = "vm_memory_map_%p.txt";
 
 SystemDumpMapDCmd::SystemDumpMapDCmd(outputStream* output, bool heap) :
   DCmdWithParser(output, heap),
-  _filename("-F", "file path", "STRING", false, default_filename) {
+  _filename("-F", "file path", "FILE", false, default_filename) {
   _dcmdparser.add_dcmd_option(&_filename);
 }
 
 void SystemDumpMapDCmd::execute(DCmdSource source, TRAPS) {
-  stringStream defaultname;
-  const char* name = nullptr;
-  if (_filename.is_set()) {
-    name = _filename.value();
-  } else {
-    defaultname.print("vm_memory_map_%d.txt", os::current_process_id());
-    name = defaultname.base();
-  }
+  const char* name = _filename.value();
   fileStream fs(name);
   if (fs.is_open()) {
     if (!MemTracker::enabled()) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2024, 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
@@ -190,6 +190,10 @@ protected static OutputAnalyzer test(String fileName, long pid,
         PidJcmdExecutor cmdExecutor = new PidJcmdExecutor(String.valueOf(pid));
         OutputAnalyzer output = cmdExecutor.execute(jcmd, true/*silent*/);
 
+        if (archiveFileName.contains("%p")) {
+            archiveFileName = archiveFileName.replace("%p", "%d").formatted(pid);
+        }
+
         if (expectOK) {
             output.shouldHaveExitValue(0);
             checkFileExistence(archiveFileName, true);

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2024, 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
@@ -87,6 +87,13 @@ static void test() throws Exception {
         }
         app.stopApp();
 
+        // Test static dump with file name containing %p
+        print2ln(test_count++ + " Test static dump with given file name containing %p.");
+        app = createLingeredApp("-cp", allJars);
+        pid = app.getPid();
+        test("%p.jsa", pid, noBoot, EXPECT_PASS, STATIC_MESSAGES);
+        app.stopApp();
+
         //  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(test_count++ + " Test static dump with flags with which dumping should fail.");

test/hotspot/jtreg/serviceability/ParserTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, 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
@@ -33,6 +33,7 @@
 
 import java.math.BigInteger;
 
+import jdk.test.lib.process.ProcessTools;
 import jdk.test.whitebox.parser.DiagnosticCommand;
 import jdk.test.whitebox.parser.DiagnosticCommand.DiagnosticArgumentType;
 import jdk.test.whitebox.WhiteBox;
@@ -49,6 +50,7 @@ public ParserTest() throws Exception {
         testQuotes();
         testMemorySize();
         testSingleLetterArg();
+        testFileName();
     }
 
     public static void main(String... args) throws Exception  {
@@ -159,6 +161,33 @@ public void testSingleLetterArg() throws Exception {
         parse("value", "v", "flag v", ' ', args);
     }
 
+    public void testFileName() throws Exception {
+        // --- Testing options
+        long pid = ProcessTools.getProcessId();
+
+        // Test pid gets injected into %p
+        String name = "name";
+        DiagnosticCommand arg = new DiagnosticCommand(name,
+                "desc", DiagnosticArgumentType.FILE,
+                false, null);
+        DiagnosticCommand[] args = {arg};
+        parse(name, "file%d.txt".formatted(pid), name + "=file%p.txt", args);
+
+        // Test custom file name with no %p
+        parse(name, "myFile.txt", name + "=myFile.txt", args);
+
+        // --- Testing arguments
+
+        // Test pid gets injected into %p
+        arg = new DiagnosticCommand(name, "desc", DiagnosticArgumentType.FILE, true,
+                false, null);
+        args = new DiagnosticCommand[]{arg};
+        parse(name, "file%d.txt".formatted(pid), "file%p.txt", args);
+
+        // Test custom file name with no %p
+        parse(name, "myFile.txt", "myFile.txt", args);
+    }
+
     public void testMemorySize() throws Exception {
         String name = "name";
         String defaultValue = "1024";

test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, Red Hat Inc.
+ * 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 8334492
+ * @summary Test to verify jcmd accepts %p in output filenames and substitutes for PID
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ * @run driver TestJcmdPIDSubstitution
+ *
+ */
+
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+public class TestJcmdPIDSubstitution {
+
+    private static final String FILENAME = "myfile%p";
+
+    public static void main(String[] args) throws Exception {
+        verifyOutputFilenames("Thread.dump_to_file", FILENAME);
+        verifyOutputFilenames("GC.heap_dump", FILENAME);
+        verifyOutputFilenames("Compiler.perfmap", FILENAME);
+        verifyOutputFilenames("System.dump_map", "-F=%s".formatted(FILENAME));
+    }
+
+    private static void verifyOutputFilenames(String... args) throws Exception {
+        long pid = ProcessTools.getProcessId();
+        String test_dir = System.getProperty("test.dir", ".");
+        Path path = Paths.get("%s/myfile%d".formatted(test_dir, pid));
+        OutputAnalyzer output = JcmdBase.jcmd(args);
+        output.shouldHaveExitValue(0);
+        if (Files.exists(path)) {
+            Files.delete(path);
+        } else {
+            throw new Exception("File %s was not created as expected for diagnostic cmd %s".formatted(path, args[0]));
+        }
+    }
+}

test/lib/jdk/test/whitebox/parser/DiagnosticCommand.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, 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
@@ -26,7 +26,7 @@
 public class DiagnosticCommand {
 
     public enum DiagnosticArgumentType {
-        JLONG, BOOLEAN, STRING, NANOTIME, STRINGARRAY, MEMORYSIZE
+        JLONG, BOOLEAN, STRING, NANOTIME, STRINGARRAY, MEMORYSIZE, FILE
     }
 
     private String name;