On Mon, 7 Apr 2025 09:05:34 GMT, Kevin Walls <kev...@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
