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

[Thomas Stuefe]

On Mon, 22 Jul 2024 20:03:08 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:
> 
>   Error messaging format

Slowly getting there... :)

src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 79:

> 77:      DCmdArgument<char*>* argument = new DCmdArgument<char*>(
> 78:      name, desc,
> 79:      "STRING", mandatory, default_value);

I would revert all these style-only changes and just keep the functional one (addition of FileArgument handling). 

Let's keep this for a follow-up.

src/hotspot/share/services/diagnosticArgument.cpp line 376:

> 374:       THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), error_msg.base());
> 375:     }
> 376:   }

The realloc here is a bit pointless since if `_value._name` is set, it already points to a buffer of size JVM_MAXPATHLEN.

I would either one of these two:

- either inline the buffer into FileArgument as I wrote earlier; no need to allocate or deallocate then.
- or, in this function, allocate if _name is not null, use existing buffer otherwise

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

> 522:   HeapDumper dumper(!_all.value() /* request GC if _all is false*/);
> 523:   dumper.dump(_filename.value()._name, output(), (int)level, _overwrite.value(),
> 524:               (uint)parallel);

Please revert style-only changes, lets keep those for follow ups.

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

> 1193: 
> 1194: void SystemDumpMapDCmd::execute(DCmdSource source, TRAPS) {
> 1195:   const char* name = _filename.value()._name;

This direct access to the member inside _filename is a bit awkward. I would make the buffer private and give the class some getters and setters, possibly like this:

class FileArgument {
  // private stuff
public:
  const char* get() const { 
    // return internal buffer
  }

  // returns true if parsing succeeded, false if not
  bool parse_value(const char* s, size_t len) {
     // call Arguments::copyexpand, target internal buffer, and return its return value
  }
}

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

PR Review: https://git.openjdk.org/jdk/pull/20198#pullrequestreview-2193132206
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1687527542
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688310305
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688264948
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1688316839