I found os::file_separator() that can be used in an absolute
pathname check. There is an isAbsolute function in the
libinstrument native code, but the code we're modifying
is in libjvm.
Here's a quick prototype of an absolute path check...
diff --git a/src/hotspot/share/services/diagnosticCommand.cpp
b/src/hotspot/share/services/diagnosticCommand.cpp
--- a/src/hotspot/share/services/diagnosticCommand.cpp
+++ b/src/hotspot/share/services/diagnosticCommand.cpp
@@ -504,13 +504,30 @@
}
void HeapDumpDCmd::execute(DCmdSource source, TRAPS) {
+ char full_dump_pathname[JVM_MAXPATHLEN];
+ if ((strncmp(_filename.value(), os::file_separator(), 1) == 0)
+#if defined(_WINDOWS)
+ || (strchr(_filename.value(), ':') != NULL)
+#endif
+ ) {
+ // Absolute path
+ strcpy(full_dump_pathname, _filename.value());
+ } else {
+ // Relative path
+ sprintf (full_dump_pathname,"%s%s%s",
+ os::get_current_directory(NULL, 0),
+ os::file_separator(),
+ _filename.value());
+ }
+
// Request a full GC before heap dump if _all is false
// This helps reduces the amount of unreachable objects in the dump
// and makes it easier to browse.
HeapDumper dumper(!_all.value() /* request GC if _all is false*/);
int res = dumper.dump(_filename.value());
if (res == 0) {
- output()->print_cr("Heap dump file created");
+ output()->print_cr("Heap dump file created [%s]",
+ full_dump_pathname);
} else {
// heap dump failed
ResourceMark rm;
@@ -518,7 +535,8 @@
if (error == NULL) {
output()->print_cr("Dump failed - reason unknown");
} else {
- output()->print_cr("%s", error);
+ output()->print_cr("%s [%s]", error,
+ full_dump_pathname);
}
}
}
On 8/29/18, 3:49 PM, Chris Plummer wrote:
On 8/29/18 12:42 PM, [email protected] wrote:
On 8/29/18 3:12 PM, Chris Plummer wrote:
On 8/29/18 11:57 AM, Gary Adams wrote:
Here are the items I'd like to discuss.
1. The current help doc only mention <filename>
as a positional argument.
The original bug was posted with an expectation
that <filename=value> should be accepted.
Do we document that we support either form
in the help message?
That's a good question. Or should we even support both forms. I
think other dcmds require filename=<filename>. If we decide to not
use your new support for positional arguments specified with
key=<value>, then we should do a better job of detecting when this
mistake is made and give an appropriate error. I think currently
(without your changes) if you specify filename=foo, you get a file
named "filename=foo".
Actually, you get the file named "filename".
The dcmd parsing already split the "key=value" parameter,
but since only a positional argument was expected it took
the <key> portion.
I like supporting either form of the argument.
It's backward compatible if people are already using it correctly.
And it's forward compatible with the syntax of the JFR key=value
expectations.
Maybe we should just document the preferred way.
Do we change the issue type from bug to rfe?
Not sure. I suppose confusing argument handling could be considered
a bug.
Actually it's not confusing. It matches what the help text describes.
The confusion was introduced when the JFR commands introduced
a different approach to parameters.
JFR is key=value and we didn't have that previously?
Chris
Do we need a CSR if the dcmd syntax is changed?
I think yes.
2. Do we have a native function that will declare a
path as absolute or relative to the current directory?
Worried a little about windows paths here.
I don't know. There probably is one to canonicalize a path.
Chris
3. Looks like minimal heap_dump testing here:
open/test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpTest.java
...
On 8/29/18, 2:04 PM, Chris Plummer wrote:
Hi Gary,
Ok. Overall looks good. A couple of comments:
You now output the path of the created file, but in a kind of
strange way. You print the CWD, and then the specified path,
whether relative or full. So it's kind of confusing because if the
user specifies a full path, they'll still see the CWD printed out.
Why not just print the full path (after combining with CWD if
needed), or maybe omit the CWD off when a full path was specified.
If determining if CWD ends up getting used is difficult, maybe
just make it more clear what you are printing is the CWD, and it
is not necessarily part of the actual filename path.
What about writing some tests cases. It would probably be pretty
easy to update some of the existing test cases to use key=<value>
where previously they just allowed <value>.
thanks,
Chris
On 8/29/18 8:10 AM, Gary Adams wrote:
Sure no problem.
I have two workspaces I use called jdk-jdb and jdk-jdk.
I have the changes in the jdk-jdb workspace for the heap
filename updates.
In the jdk-jdb workspace a launch a process that just waits for
input.
jdk/bin/java -cp ~/classes my
In the jdk-jdk workspace I issue the jcmd to talk to the java
process
running in the jdk-jdb workspace. The updated output includes
"[<current-directory>,<heap dump filename>]"
...
$ jdk/bin/jcmd 2328 GC.heap_dump filename=foo
2328:
Heap dump file created
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,foo]
$ jdk/bin/jcmd 2328 GC.heap_dump filename
2328:
Heap dump file created
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,filename]
$ jdk/bin/jcmd 2328 GC.heap_dump ffoo2
2328:
Heap dump file created
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
$ jdk/bin/jcmd 2328 GC.heap_dump ffoo2
2328:
File exists [/export/users/gradams/ws/jdk-jdb/build/linux-x64,ffoo2]
$ jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
2328:
Heap dump file created
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2]
$ jdk/bin/jcmd 2328 GC.heap_dump ~/ffoo2
2328:
File exists
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,/home/gradams/ffoo2]
$ jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
2328:
Heap dump file created
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
$ jdk/bin/jcmd 2328 GC.heap_dump /tmp/ffoo2
2328:
File exists
[/export/users/gradams/ws/jdk-jdb/build/linux-x64,/tmp/ffoo2]
$ pwd
/home/gradams/scratch/ws/jdk-jdk/build/linux-x64
On 8/29/18, 2:21 AM, Chris Plummer wrote:
Hi Gary,
What would be really useful are some before/after examples that
demonstrate what has changed from the users point of view.
thanks,
Chris
On 8/28/18 5:26 AM, Gary Adams wrote:
This message may have been lost in the vacation email backlog,
so I'm
sending it again.
On 8/9/18, 2:19 PM, Gary Adams wrote:
Thee are several reported problems using jcmd for obtaining
heap dumps.
Some users are confused by the positional argument for a filename
when a similar jfr command uses "key=value" syntax.
Some users are confused by the basic nature of the command,
where the jvm
executing the heap dump is running in a different current
directory than
the jcmd itself.
This is a proposed fix to report the current directory and the
filename
when the heap dump is successfully written or if an error
occurs. There
is also a proposed fix to handle the case when the user provided
"key=value" inputs. If the key matches the argument name, then
the
user intended the value to be used.
Issue: https://bugs.openjdk.java.net/browse/JDK-8177763
Webrev: http://cr.openjdk.java.net/~gadams/8177763/webrev.00/
