RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v2]

[Thomas Stuefe]

On Wed, 17 Jul 2024 14:02:31 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8334492](https://bugs.openjdk.org/browse/JDK-8334492) enabling jcmd diagnostic commands that issue an output file to accept the `%p` pattern in the file name and substitute it for the PID. 
>> 
>> This PR addresses the following diagnostic commands: 
>> - [x] Compiler.perfmap 
>> - [x] GC.heap_dump
>> - [x] System.dump_map
>> - [x] Thread.dump_to_file
>> - [x] VM.cds
>> 
>> Note that some jcmd diagnostic commands already enable this functionality (`JFR.configure, JFR.dump, JFR.start and JFR.stop`). 
>> 
>> I propose opening a separate issue to track updating the man page similarly to how it’s done for the JFR diagnostic commands. For example, 
>> 
>> 
>> filename         (Optional) Name of the file to which the flight recording data is
>>                    written when the recording is stopped. If no filename is given, a
>>                    filename is generated from the PID and the current date and is
>>                    placed in the directory where the process was started. The
>>                    filename may also be a directory in which case, the filename is
>>                    generated from the PID and the current date in the specified
>>                    directory. (STRING, no default value)
>> 
>>                    Note: If a filename is given, '%p' in the filename will be
>>                    replaced by the PID, and '%t' will be replaced by the time in
>>                    'yyyy_MM_dd_HH_mm_ss' format.
>> 
>> 
>> Unfortunately, per [8276265](https://bugs.openjdk.org/browse/JDK-8276265), sources for the jcmd manpage remain in Oracle internal repos so this PR can’t address that. 
>> 
>> Testing: 
>> 
>> - [x] Added test case passes. 
>> - [x] Modified existing VM.cds tests to also check for `%p` filenames. 
>> 
>> Looking forward to your comments and addressing any diagnostic commands I might have missed (if any). 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updating copyright headers

First cursory review. That is a useful feature

- In all cases: please, in case of an error, don't THROW, don't do `warning`. Instead, just print to the `output()` of the DCmd. You want an error to appear to the user of the dcmd - so, to stdout or stderr of the jcmd process issuing the command. Not an exception in the target JVM process, nor a warning in the target JVM stderr stream

- Can you give us a variant of `Arguments::copy_expand_pid` that receives a zero-terminated const char* as input so that we can avoid having to pass in the length of the input each time?

- when passing in output buffers to functions, try to use sizeof(buffer) instead of repeating the buffer size. Then, one can change the size of the buffer array without having to modify using calls (but aware: pitfall, sizeof(char[]) vs sizeof(char*))

src/hotspot/share/code/codeCache.cpp line 1796:

> 1794:   // Perf expects to find the map file at /tmp/perf-<pid>.map
> 1795:   // if the file name is not specified.
> 1796:   char fname[JVM_MAXPATHLEN];

Good to see this gone, the old code implicitly relied on: max pid len -2147483647 = 11 chars, + length of "/tmp/perf-.map" not overflowing 32, which cuts a bit close to the bone.

src/hotspot/share/code/codeCache.cpp line 1800:

> 1798:     jio_snprintf(fname, sizeof(fname), "/tmp/perf-%d.map",
> 1799:                  os::current_process_id());
> 1800:   }

Arguably one could just do 

constexpr char[] filename_default = "/tmp/perf-%p.map";
Arguments::copy_expand_pid(filename == nullptr ? filename_default : filename, .....);

src/hotspot/share/services/diagnosticCommand.cpp line 525:

> 523:     stringStream msg;
> 524:     msg.print("Invalid file path name specified: %s", _filename.value());
> 525:     THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), msg.base());

Why throw? Why not just print an error message to the output() stream and return?

src/hotspot/share/services/diagnosticCommand.cpp line 1059:

> 1057:       fileh = java_lang_String::create_from_str(fname, CHECK);
> 1058:     } else {
> 1059:       warning("Invalid file path name specified, fall back to default name");

`warning` prints a warning to the stdout of the JVM process. You don't want that; you want a warning to the issuer of the dcmd, which is another - possibly even remote - process. Write errors to `output()`, instead.

src/hotspot/share/services/diagnosticCommand.cpp line 1138:

> 1136:     stringStream msg;
> 1137:     msg.print("Invalid file path name specified: %s", _filepath.value());
> 1138:     THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), msg.base());

write to output() and return instead of throwing

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

PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2183023385
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681109673
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681115247
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681118969
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681124783
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1681125914