RFR: 8353727: HeapDumpPath doesn't expand %p

[Chris Plummer]

On Mon, 7 Apr 2025 09:05:34 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

> This is a long-standing oversight: HeapDumpPath does not recognise %p for pid expansion.
> The default filename uses a pid (e.g. java_pid1676937.hprof) but HeapDumpPath does not.
> It has always done a manual "root plus pid plus extension" on the default filename only, and
> should move to using Argument::copy_expand_pid() like we do with other such filenames.
> 
> 
> We also assumed the default filename is not a directory (which is very very likely, but doesn't have to be true).

src/hotspot/share/services/heapDumper.cpp line 2772:

> 2770: 
> 2771:     // Set base path (name or directory, default or custom, without seq no), doing %p substitution.
> 2772:     const char *path_src = (HeapDumpPath && HeapDumpPath[0] != '\0') ? HeapDumpPath : dump_file_name;

Should be `HeapDumpPath != nullptr`.

src/hotspot/share/services/heapDumper.cpp line 2792:

> 2790:       // Path is a directory.  Append the default name, with %p substitution.  Use my_path temporarily.
> 2791:       if (!Arguments::copy_expand_pid(dump_file_name, strlen(dump_file_name), my_path, JVM_MAXPATHLEN)) {
> 2792:         warning("Cannot create heap dump file.  HeapDumpPath is too long.");

What is going to be the end result of this? A truncated file name?

test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java line 101:

> 99:         File dump = new File(heapdumpFilename);
> 100:         Asserts.assertTrue(dump.exists() && dump.isFile(), "Could not find dump file " + dump.getAbsolutePath());
> 101: 

I think you can remove this empty line, especially since you don't have one in the similar code below.

test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java line 113:

> 111:                 TestHeapDumpOnOutOfMemoryError.class.getName(), type);
> 112: 
> 113:         Process proc = pb.start();

No need differ from the above code here. You can just use OutputAnalyzer.pid() to get the pid in the code below.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2032238479
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2032243030
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2032233983
PR Review Comment: https://git.openjdk.org/jdk/pull/24482#discussion_r2032234654