From f5c9e8f1229f3aa00743119a2dee3e15d57048d8 Mon Sep 17 00:00:00 2001 From: Sonia Zaldana Calles Date: Tue, 30 Jul 2024 18:40:37 +0000 Subject: [PATCH] 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID Reviewed-by: kevinw, stuefe, lmesnik --- src/hotspot/share/code/codeCache.cpp | 14 ++-- src/hotspot/share/code/codeCache.hpp | 4 ++ .../share/prims/wbtestmethods/parserTests.cpp | 10 ++- .../share/services/diagnosticArgument.cpp | 17 +++-- .../share/services/diagnosticArgument.hpp | 3 +- .../share/services/diagnosticCommand.cpp | 32 +++------ .../cds/appcds/jcmd/JCmdTestDumpBase.java | 6 +- .../cds/appcds/jcmd/JCmdTestStaticDump.java | 9 ++- .../jtreg/serviceability/ParserTest.java | 31 ++++++++- .../tools/jcmd/TestJcmdPIDSubstitution.java | 66 +++++++++++++++++++ .../whitebox/parser/DiagnosticCommand.java | 4 +- 11 files changed, 157 insertions(+), 39 deletions(-) create mode 100644 test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java diff --git a/src/hotspot/share/code/codeCache.cpp b/src/hotspot/share/code/codeCache.cpp index ea01dd75958..a491dd62f07 100644 --- a/src/hotspot/share/code/codeCache.cpp +++ b/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-.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); diff --git a/src/hotspot/share/code/codeCache.hpp b/src/hotspot/share/code/codeCache.hpp index 565f600453e..b724268b650 100644 --- a/src/hotspot/share/code/codeCache.hpp +++ b/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; diff --git a/src/hotspot/share/prims/wbtestmethods/parserTests.cpp b/src/hotspot/share/prims/wbtestmethods/parserTests.cpp index 2845aa9aa10..efcf2b070d0 100644 --- a/src/hotspot/share/prims/wbtestmethods/parserTests.cpp +++ b/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* argument = + new DCmdArgument(name, desc, "FILE", mandatory); + if (isarg) { + parser->add_dcmd_argument(argument); + } else { + parser->add_dcmd_option(argument); + } } } diff --git a/src/hotspot/share/services/diagnosticArgument.cpp b/src/hotspot/share/services/diagnosticArgument.cpp index 94f2d3e1eb1..5fd565a605a 100644 --- a/src/hotspot/share/services/diagnosticArgument.cpp +++ b/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::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"); + } } } diff --git a/src/hotspot/share/services/diagnosticArgument.hpp b/src/hotspot/share/services/diagnosticArgument.hpp index c9683ce4a21..1451ea34f86 100644 --- a/src/hotspot/share/services/diagnosticArgument.hpp +++ b/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" diff --git a/src/hotspot/share/services/diagnosticCommand.cpp b/src/hotspot/share/services/diagnosticCommand.cpp index e6328e95abd..7a2c0838146 100644 --- a/src/hotspot/share/services/diagnosticCommand.cpp +++ b/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-.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_.jsa" +#define DEFAULT_CDS_ARCHIVE_FILENAME "java_pid%p_.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_.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()) { diff --git a/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestDumpBase.java b/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestDumpBase.java index ad7b8e97b81..eedefaa1ba1 100644 --- a/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestDumpBase.java +++ b/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 @@ public abstract class JCmdTestDumpBase { 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); diff --git a/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestStaticDump.java b/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestStaticDump.java index a95eaf672c1..08ccf9b7f2a 100644 --- a/test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTestStaticDump.java +++ b/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 @@ public class JCmdTestStaticDump extends JCmdTestDumpBase { } 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."); diff --git a/test/hotspot/jtreg/serviceability/ParserTest.java b/test/hotspot/jtreg/serviceability/ParserTest.java index 70d666d2ed0..7c304af243c 100644 --- a/test/hotspot/jtreg/serviceability/ParserTest.java +++ b/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 class ParserTest { testQuotes(); testMemorySize(); testSingleLetterArg(); + testFileName(); } public static void main(String... args) throws Exception { @@ -159,6 +161,33 @@ public class ParserTest { 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"; diff --git a/test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java b/test/jdk/sun/tools/jcmd/TestJcmdPIDSubstitution.java new file mode 100644 index 00000000000..f462ea1a52b --- /dev/null +++ b/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])); + } + } +} diff --git a/test/lib/jdk/test/whitebox/parser/DiagnosticCommand.java b/test/lib/jdk/test/whitebox/parser/DiagnosticCommand.java index adb699bac6a..6b7efa2dc87 100644 --- a/test/lib/jdk/test/whitebox/parser/DiagnosticCommand.java +++ b/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 @@ package jdk.test.whitebox.parser; public class DiagnosticCommand { public enum DiagnosticArgumentType { - JLONG, BOOLEAN, STRING, NANOTIME, STRINGARRAY, MEMORYSIZE + JLONG, BOOLEAN, STRING, NANOTIME, STRINGARRAY, MEMORYSIZE, FILE } private String name;