Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Serguei,
I've inlined below the corresponding code in HeapDumper::dump() that
covers the deleted output functionality from attachListener.cpp. Note
it's not exactly the same, but I think in the end it includes at least
the same info in all cases, and in some cases more info:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
2009 out->print_cr("Heap dump file created [" JULONG_FORMAT "
bytes in %3.3f secs]",
2010 writer.bytes_written(), timer()->seconds());
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
1986 out->print_cr("Unable to create %s: %s", path,
1987 (error() != NULL) ? error() : "reason unknown");
- } else {
- out->print_cr("%s", error);
- }
2012 out->print_cr("Dump file is incomplete: %s", writer.error());
- }
+ dumper.dump(op->arg(0), out);
Chris
On 11/8/19 6:31 PM, serguei.spit...@oracle.com wrote:
Okay, thanks.
Agreed, this aspect was clear to me.
The deleted fragments are about printing the summarizing conclusion
about the dumping which is not present in the HeapDumper::dump().
I expected it to be moved into the HeapDumper::dump() but it was not.
So, I wonder if there was such an intention.
Thanks,
Serguei
On 11/8/19 18:14, Chris Plummer wrote:
He also added the outputStream argument to HeapDumper::dump(). Both
of the sections of code below already called dump(), and now they do
so with the added outputStream argument. HeapDumper::dump() has been
modified to print on the outputStream rather than to the tty.
Chris
On 11/8/19 5:02 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
I'm a little bit confused.
The Ralf's change in the HeapDumper::dump() is just replacement of
'tty' occurrences with 'out',
so the change has not moved the deleted code in these files into the
HeapDumper::dump().
Probably, you wanted to say that the pre-existed error messages
printed in the HeapDumper::dump() are enough.
This would explain why the code is deleted.
Just wanted a bit of clarification from Ralf to make sure it is the
case.
Thanks,
Serguei
On 11/8/19 16:42, Chris Plummer wrote:
Hi Ralf,
Also looks good to me.
Serguei, the removed code is consolidated into HeapDumper::dump().
thanks,
Chris
On 11/8/19 4:25 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were
removed?
Is it because this information is not that useful or there is some
other motivation?
attachListener.cpp:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
- } else {
- out->print_cr("%s", error);
- }
- }
diagnisticCommand.cpp
- if (res == 0) {
- output()->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
- } else {
- output()->print_cr("%s", error);
- }
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump()
command to an optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of
the attach framework partly recreated the output by hand, but
missing some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Okay, thanks.
Agreed, this aspect was clear to me.
The deleted fragments are about printing the summarizing conclusion
about the dumping which is not present in the HeapDumper::dump().
I expected it to be moved into the HeapDumper::dump() but it was not.
So, I wonder if there was such an intention.
Thanks,
Serguei
On 11/8/19 18:14, Chris Plummer wrote:
He also added the outputStream argument to HeapDumper::dump(). Both of
the sections of code below already called dump(), and now they do so
with the added outputStream argument. HeapDumper::dump() has been
modified to print on the outputStream rather than to the tty.
Chris
On 11/8/19 5:02 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
I'm a little bit confused.
The Ralf's change in the HeapDumper::dump() is just replacement of
'tty' occurrences with 'out',
so the change has not moved the deleted code in these files into the
HeapDumper::dump().
Probably, you wanted to say that the pre-existed error messages
printed in the HeapDumper::dump() are enough.
This would explain why the code is deleted.
Just wanted a bit of clarification from Ralf to make sure it is the
case.
Thanks,
Serguei
On 11/8/19 16:42, Chris Plummer wrote:
Hi Ralf,
Also looks good to me.
Serguei, the removed code is consolidated into HeapDumper::dump().
thanks,
Chris
On 11/8/19 4:25 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were
removed?
Is it because this information is not that useful or there is some
other motivation?
attachListener.cpp:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
- } else {
- out->print_cr("%s", error);
- }
- }
diagnisticCommand.cpp
- if (res == 0) {
- output()->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
- } else {
- output()->print_cr("%s", error);
- }
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump() command
to an optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of
the attach framework partly recreated the output by hand, but
missing some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
He also added the outputStream argument to HeapDumper::dump(). Both of
the sections of code below already called dump(), and now they do so
with the added outputStream argument. HeapDumper::dump() has been
modified to print on the outputStream rather than to the tty.
Chris
On 11/8/19 5:02 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
I'm a little bit confused.
The Ralf's change in the HeapDumper::dump() is just replacement of
'tty' occurrences with 'out',
so the change has not moved the deleted code in these files into the
HeapDumper::dump().
Probably, you wanted to say that the pre-existed error messages
printed in the HeapDumper::dump() are enough.
This would explain why the code is deleted.
Just wanted a bit of clarification from Ralf to make sure it is the case.
Thanks,
Serguei
On 11/8/19 16:42, Chris Plummer wrote:
Hi Ralf,
Also looks good to me.
Serguei, the removed code is consolidated into HeapDumper::dump().
thanks,
Chris
On 11/8/19 4:25 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were
removed?
Is it because this information is not that useful or there is some
other motivation?
attachListener.cpp:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
- } else {
- out->print_cr("%s", error);
- }
- }
diagnisticCommand.cpp
- if (res == 0) {
- output()->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
- } else {
- output()->print_cr("%s", error);
- }
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump() command
to an optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of the
attach framework partly recreated the output by hand, but missing
some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"
On 11/08/2019 16:55, Chris Plummer wrote: Hi Alex, Comments below: On 11/8/19 4:39 PM, Alex Menkov wrote: On 11/08/2019 15:22, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8215196 webrev: http://cr.openjdk.java.net/~amenkov/jdk14/popframe_args/webrev/ I don't really see a resolution in the JDK-8215196 comments as to what is actually broken. Are we sure we want to fix this in the test, and not require different behavior by the compiler (and also clarify the spec)? In the test activeMeth method changes its arguments values and then don't use them later. I think dropping useless code is good compiler optimization and I'd prefer to not restrict to do the optimization. Currently PopFrame is disabled with JVMCI by [1], so for testing I reverted [1] changes. Just to be clear - I temporary reverted [1] for test runs. The description for JDK-8218025 says that the intention is to only disable these capabilities for JDK12. Is there a CR to re-enabled them? https://bugs.openjdk.java.net/browse/JDK-8218885 Unfortunately the problem why the capabilities were disabled are still unresolved and looks like won't be resolved in 14, so for now it's targeted to tbd. --alex thanks, Chris --alex [1] https://bugs.openjdk.java.net/browse/JDK-8218025 --alex
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Chris,
I'm a little bit confused.
The Ralf's change in the HeapDumper::dump() is just replacement of 'tty'
occurrences with 'out',
so the change has not moved the deleted code in these files into the
HeapDumper::dump().
Probably, you wanted to say that the pre-existed error messages printed
in the HeapDumper::dump() are enough.
This would explain why the code is deleted.
Just wanted a bit of clarification from Ralf to make sure it is the case.
Thanks,
Serguei
On 11/8/19 16:42, Chris Plummer wrote:
Hi Ralf,
Also looks good to me.
Serguei, the removed code is consolidated into HeapDumper::dump().
thanks,
Chris
On 11/8/19 4:25 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were removed?
Is it because this information is not that useful or there is some
other motivation?
attachListener.cpp:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
- } else {
- out->print_cr("%s", error);
- }
- }
diagnisticCommand.cpp
- if (res == 0) {
- output()->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
- } else {
- output()->print_cr("%s", error);
- }
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump() command
to an optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of the
attach framework partly recreated the output by hand, but missing
some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"
Hi Alex, Comments below: On 11/8/19 4:39 PM, Alex Menkov wrote: On 11/08/2019 15:22, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8215196 webrev: http://cr.openjdk.java.net/~amenkov/jdk14/popframe_args/webrev/ I don't really see a resolution in the JDK-8215196 comments as to what is actually broken. Are we sure we want to fix this in the test, and not require different behavior by the compiler (and also clarify the spec)? Currently PopFrame is disabled with JVMCI by [1], so for testing I reverted [1] changes. Just to be clear - I temporary reverted [1] for test runs. The description for JDK-8218025 says that the intention is to only disable these capabilities for JDK12. Is there a CR to re-enabled them? thanks, Chris --alex [1] https://bugs.openjdk.java.net/browse/JDK-8218025 --alex
Re: RFR: 8233868: Unproblem list sun/tools/jstat/jstatClassloadOutput1.sh
+1 On 11/8/19 4:40 PM, Alex Menkov wrote: LGTM --alex On 11/08/2019 14:58, Daniil Titov wrote: Please a review a changeset below that removes test sun/tools/jstat/jstatClassloadOutput1.sh from test/jdk/ProblemList.txt. diff -r f92ef5d182b5 test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt Fri Nov 08 11:41:17 2019 -0500 +++ b/test/jdk/ProblemList.txt Fri Nov 08 22:37:11 2019 + @@ -861,7 +861,6 @@ # svc_tools -sun/tools/jstat/jstatClassloadOutput1.sh 8173942 generic-all sun/tools/jhsdb/BasicLauncherTest.java 8193639,8211767 solaris-all,linux-ppc64,linux-ppc64le sun/tools/jhsdb/HeapDumpTest.java 8193639 solaris-all sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java 8230731,8001227 windows-all This test was added in ProblemList.txt in [1] but this issue is no longer reproducible in JDK 14 and the test runs fine in Mach5. Mach5 tests for tier1,tier2, and tier3 successfully passed. [1] https://bugs.openjdk.java.net/browse/JDK-8173942 Thanks. Daniil
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Ralf,
Also looks good to me.
Serguei, the removed code is consolidated into HeapDumper::dump().
thanks,
Chris
On 11/8/19 4:25 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were removed?
Is it because this information is not that useful or there is some
other motivation?
attachListener.cpp:
- int res = dumper.dump(op->arg(0));
- if (res == 0) {
- out->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- out->print_cr("Dump failed - reason unknown");
- } else {
- out->print_cr("%s", error);
- }
- }
diagnisticCommand.cpp
- if (res == 0) {
- output()->print_cr("Heap dump file created");
- } else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
- } else {
- output()->print_cr("%s", error);
- }
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump() command to an
optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of the attach
framework partly recreated the output by hand, but missing some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport:https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev:http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR: 8233868: Unproblem list sun/tools/jstat/jstatClassloadOutput1.sh
LGTM --alex On 11/08/2019 14:58, Daniil Titov wrote: Please a review a changeset below that removes test sun/tools/jstat/jstatClassloadOutput1.sh from test/jdk/ProblemList.txt. diff -r f92ef5d182b5 test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt Fri Nov 08 11:41:17 2019 -0500 +++ b/test/jdk/ProblemList.txt Fri Nov 08 22:37:11 2019 + @@ -861,7 +861,6 @@ # svc_tools -sun/tools/jstat/jstatClassloadOutput1.sh8173942 generic-all sun/tools/jhsdb/BasicLauncherTest.java 8193639,8211767 solaris-all,linux-ppc64,linux-ppc64le sun/tools/jhsdb/HeapDumpTest.java 8193639 solaris-all sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java 8230731,8001227 windows-all This test was added in ProblemList.txt in [1] but this issue is no longer reproducible in JDK 14 and the test runs fine in Mach5. Mach5 tests for tier1,tier2, and tier3 successfully passed. [1] https://bugs.openjdk.java.net/browse/JDK-8173942 Thanks. Daniil
Re: RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"
On 11/08/2019 15:22, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8215196 webrev: http://cr.openjdk.java.net/~amenkov/jdk14/popframe_args/webrev/ Currently PopFrame is disabled with JVMCI by [1], so for testing I reverted [1] changes. Just to be clear - I temporary reverted [1] for test runs. --alex [1] https://bugs.openjdk.java.net/browse/JDK-8218025 --alex
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Ralf,
The fix looks Okay in general.
A couple of questions.
The dumper.dump() returns int value.
Returned value is not used anymore in the attachListener.cpp and
diagnisticCommand.cpp.
Is it still used somewhere else or we can replace it with void?
Could you explain a little bit why the following fragments were
removed?
Is it because this information is not that useful or there is
some other motivation?
attachListener.cpp:
-int res = dumper.dump(op->arg(0));
-if (res == 0) {
- out->print_cr("Heap dump file created");
-} else {
- // heap dump failed
- ResourceMark rm;
- char* error = dumper.error_as_C_string();
- if (error == NULL) {
-out->print_cr("Dump failed - reason unknown");
- } else {
-out->print_cr("%s", error);
- }
-}
diagnisticCommand.cpp
- if (res == 0) {
-output()->print_cr("Heap dump file created");
- } else {
-// heap dump failed
-ResourceMark rm;
-char* error = dumper.error_as_C_string();
-if (error == NULL) {
- output()->print_cr("Dump failed - reason unknown");
-} else {
- output()->print_cr("%s", error);
-}
- }
Thanks,
Serguei
On 11/8/19 03:56, Schmelter, Ralf wrote:
This change forwards the output from the HeapDumper.dump() command to an optional output stream supplied by the caller.
Until now the diagnositic command and the "dumpheap" command of the attach framework partly recreated the output by hand, but missing some information.
Old output:
Heap dump file created
New output:
Dumping heap to test.hprof ...
Heap dump file created [9719330384 bytes in 27,759 secs]
In addition to getting this improved information, it saves code too.
Bugreport: https://bugs.openjdk.java.net/browse/JDK-8233790
Webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/
Best regards,
Ralf
Re: RFR(S): JDK-8231635: SA Stackwalking code stuck in BasicTypeDataBase.findDynamicTypeForAddress()
Hi Chris,
This seems to be a good fix to have in any case.
This check and bail out is right thing to do and should not break anything.
I understand, this also fixes the test failures.
I only had some experience a long time ago with the support of pstack
and DTrace jstack action implementation which also does such SP
recovering because the ebp can be used by JIT compiler as a general
purpose register. There is no such a problem on sparc.
Thanks,
Serguei
On 11/7/19 14:01, Chris Plummer wrote:
Hi,
Please review the following fix for JDK-8231635:
https://bugs.openjdk.java.net/browse/JDK-8231635
http://cr.openjdk.java.net/~cjplummer/8231635/webrev.00/
I've tried to explain below to the best of my ability what's is going
on, but keep in mind that I basically had no background in this area
before looking into this CR, so this is all new to me. Please feel
free to chime in with corrections to my explanation, or any additional
insight that might help to further understanding of this code.
When doing a thread stack dump, SA has to figure out the SP for the
current frame when it may not in fact be stored anywhere. So it goes
through a series of guesses, starting with the current value of SP.
See AMD64CurrentFrameGuess.run():
Address sp = context.getRegisterAsAddress(AMD64ThreadContext.RSP);
There are a number of checks done to see if this is the SP for the
actual current frame, one of the checks being (and kind of a last
resort) to follow the frame links and see if they eventually lead to
the first entry frame:
while (frame != null) {
if (frame.isEntryFrame() && frame.entryFrameIsFirst()) {
...
return true;
}
frame = frame.sender(map);
}
If this fails, there is an outer loop to try the next address:
for (long offset = 0;
offset < regionInBytesToSearch;
offset += vm.getAddressSize()) {
Note that offset is added to the initial SP value that was fetched
from RSP. This approach is fraught with danger, because SP could be
incorrect, and you can easily follow a bad frame link to an invalid
address. So the body of this loop is in a try block that catches all
Exceptions, and simply retries with the next offset if one is caught.
Exceptions could be ones like UnalignedAddressException or
UnmappedAddressException.
The bug in question turns up with the following harmless looking line:
frame = frame.sender(map);
This is fine if you know that "frame" is valid, but what if it is not
(which is very commonly the case). The frame values (SP, FP, and PC)
in the returned frame could be just about anything, including being
the same as the previous frame. This is what will happen if the SP
stored in "frame" is the same as the SP that was used to initialize
"frame" in the first place. This can certainly happen when SP is not
valid to start with, and is indeed what caused this bug. The end
result is the inner while loop gets stuck in an infinite loop
traversing the same frame. So the fix is to add a check for this to
make sure to break out of the while loop if this happens. Initially I
did this with an Address.equal() call, and that seemed to fix the
problem, but then I realized it would be possible to traverse through
one or more sender frames and eventually end up returning to a
previously visited frame, thus still an infinite loop. So I decided on
checking for Address.lessThanOrEqual() instead since the send frame's
SP should always be greater than the current frame's (referred to as
oldFrame) SP. As long as we always move in one direction (towards a
higher frame address), you can't have an infinite loop in this code.
I applied this fix to x86. Although not tested, it is built (all
platform support is always built with SA). The x86 and amd64 versions
are identical except for x86/amd64 references, so I thought it best to
go ahead and do the update to x86. I did not touch ppc, but would be
willing to update if someone passes along a fix that is tested.
One final bit of clarification. The bug synopsis mentions getting
stuck in BasicTypeDataBase.findDynamicTypeForAddress(). This turns out
to not actually be the case, but every stack trace I initially looked
when I filed this CR was showing the thread being in this frame and at
the same line number. This appears to be the next available safepoint
where the thread can be suspended for stack dumping. When debugging
this some more and adding a lot of println() calls in a lot of
different locations, I started to see different frames in the
stacktrace, presumably because the println() calls where adding
additional safepoints.
thanks,
Chris
RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"
Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8215196 webrev: http://cr.openjdk.java.net/~amenkov/jdk14/popframe_args/webrev/ Currently PopFrame is disabled with JVMCI by [1], so for testing I reverted [1] changes. [1] https://bugs.openjdk.java.net/browse/JDK-8218025 --alex
RFR: 8233868: Unproblem list sun/tools/jstat/jstatClassloadOutput1.sh
Please a review a changeset below that removes test sun/tools/jstat/jstatClassloadOutput1.sh from test/jdk/ProblemList.txt. diff -r f92ef5d182b5 test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt Fri Nov 08 11:41:17 2019 -0500 +++ b/test/jdk/ProblemList.txt Fri Nov 08 22:37:11 2019 + @@ -861,7 +861,6 @@ # svc_tools -sun/tools/jstat/jstatClassloadOutput1.sh8173942 generic-all sun/tools/jhsdb/BasicLauncherTest.java 8193639,8211767 solaris-all,linux-ppc64,linux-ppc64le sun/tools/jhsdb/HeapDumpTest.java 8193639 solaris-all sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java 8230731,8001227 windows-all This test was added in ProblemList.txt in [1] but this issue is no longer reproducible in JDK 14 and the test runs fine in Mach5. Mach5 tests for tier1,tier2, and tier3 successfully passed. [1] https://bugs.openjdk.java.net/browse/JDK-8173942 Thanks. Daniil
Re: RFR(S): JDK-8231635: SA Stackwalking code stuck in BasicTypeDataBase.findDynamicTypeForAddress()
Ping!
On 11/7/19 2:01 PM, Chris Plummer wrote:
Hi,
Please review the following fix for JDK-8231635:
https://bugs.openjdk.java.net/browse/JDK-8231635
http://cr.openjdk.java.net/~cjplummer/8231635/webrev.00/
I've tried to explain below to the best of my ability what's is going
on, but keep in mind that I basically had no background in this area
before looking into this CR, so this is all new to me. Please feel
free to chime in with corrections to my explanation, or any additional
insight that might help to further understanding of this code.
When doing a thread stack dump, SA has to figure out the SP for the
current frame when it may not in fact be stored anywhere. So it goes
through a series of guesses, starting with the current value of SP.
See AMD64CurrentFrameGuess.run():
Address sp = context.getRegisterAsAddress(AMD64ThreadContext.RSP);
There are a number of checks done to see if this is the SP for the
actual current frame, one of the checks being (and kind of a last
resort) to follow the frame links and see if they eventually lead to
the first entry frame:
while (frame != null) {
if (frame.isEntryFrame() && frame.entryFrameIsFirst()) {
...
return true;
}
frame = frame.sender(map);
}
If this fails, there is an outer loop to try the next address:
for (long offset = 0;
offset < regionInBytesToSearch;
offset += vm.getAddressSize()) {
Note that offset is added to the initial SP value that was fetched
from RSP. This approach is fraught with danger, because SP could be
incorrect, and you can easily follow a bad frame link to an invalid
address. So the body of this loop is in a try block that catches all
Exceptions, and simply retries with the next offset if one is caught.
Exceptions could be ones like UnalignedAddressException or
UnmappedAddressException.
The bug in question turns up with the following harmless looking line:
frame = frame.sender(map);
This is fine if you know that "frame" is valid, but what if it is not
(which is very commonly the case). The frame values (SP, FP, and PC)
in the returned frame could be just about anything, including being
the same as the previous frame. This is what will happen if the SP
stored in "frame" is the same as the SP that was used to initialize
"frame" in the first place. This can certainly happen when SP is not
valid to start with, and is indeed what caused this bug. The end
result is the inner while loop gets stuck in an infinite loop
traversing the same frame. So the fix is to add a check for this to
make sure to break out of the while loop if this happens. Initially I
did this with an Address.equal() call, and that seemed to fix the
problem, but then I realized it would be possible to traverse through
one or more sender frames and eventually end up returning to a
previously visited frame, thus still an infinite loop. So I decided on
checking for Address.lessThanOrEqual() instead since the send frame's
SP should always be greater than the current frame's (referred to as
oldFrame) SP. As long as we always move in one direction (towards a
higher frame address), you can't have an infinite loop in this code.
I applied this fix to x86. Although not tested, it is built (all
platform support is always built with SA). The x86 and amd64 versions
are identical except for x86/amd64 references, so I thought it best to
go ahead and do the update to x86. I did not touch ppc, but would be
willing to update if someone passes along a fix that is tested.
One final bit of clarification. The bug synopsis mentions getting
stuck in BasicTypeDataBase.findDynamicTypeForAddress(). This turns out
to not actually be the case, but every stack trace I initially looked
when I filed this CR was showing the thread being in this frame and at
the same line number. This appears to be the next available safepoint
where the thread can be suspended for stack dumping. When debugging
this some more and adding a lot of println() calls in a lot of
different locations, I started to see different frames in the
stacktrace, presumably because the println() calls where adding
additional safepoints.
thanks,
Chris
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Ralf, On Fri, Nov 8, 2019 at 4:45 PM Schmelter, Ralf wrote: > Hi Thomas, > > thanks for the review. > > > > Can you please add a comment about the new parameter? E.g. "optional > outputStream to which progress- and error messages will be written". > > Will do. > > > > - in HeapDumper::dump(): while you are on it could you please initialize > my_path to NULL at function start? > > Do you mean the HeapDumper::dump_heap() method? That seems unrelated to my > change. And I would prefer to get a warning about a potentially > uninitialized use. > > Yes its unrelated but since you are in the area... But okay, I leave it up to you. > > > You can actually get rid of HeapDumper::error_as_C_string() altogher now. > > I thought about it, but the message is more than just the error text (you > would get the "Dumping heap to .." part) and is multiple lines. This > seems not fitting for an exception message. > > Okay. > > > Did you test that no jtreg tests fall over the changed output? e.g. > test/jdk/sun/tools/jmap/BasicJMapTest.java, or whatever is under > hotspot/jtreg/serviceability/jcmd > > I've checked them. They work because they just check that "Heap dump file > created" is contained in the output, which is still the case or don't check > the output at all (the dcmd tests). > > Okay. > Best regards, > Ralf > Okay. If you only add the comment I do not need to see a new webrev. Cheers, Thomas
RE: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Thomas, thanks for the review. > Can you please add a comment about the new parameter? E.g. "optional > outputStream to which progress- and error messages will be written". Will do. > - in HeapDumper::dump(): while you are on it could you please initialize > my_path to NULL at function start? Do you mean the HeapDumper::dump_heap() method? That seems unrelated to my change. And I would prefer to get a warning about a potentially uninitialized use. > You can actually get rid of HeapDumper::error_as_C_string() altogher now. I thought about it, but the message is more than just the error text (you would get the "Dumping heap to .." part) and is multiple lines. This seems not fitting for an exception message. > Did you test that no jtreg tests fall over the changed output? e.g. > test/jdk/sun/tools/jmap/BasicJMapTest.java, or whatever is under > hotspot/jtreg/serviceability/jcmd I've checked them. They work because they just check that "Heap dump file created" is contained in the output, which is still the case or don't check the output at all (the dcmd tests). Best regards, Ralf
Re: RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
Hi Ralf, this makes sense. Some small remarks: --- heapDumper.hpp // dumps the heap to the specified file, returns 0 if success. - int dump(const char* path); + int dump(const char* path, outputStream* out = NULL); Can you please add a comment about the new parameter? E.g. "optional outputStream to which progress- and error messages will be written". -- heapDumper.cpp - in HeapDumper::dump(): while you are on it could you please initialize my_path to NULL at function start? --- You can actually get rid of HeapDumper::error_as_C_string() altogher now. Last remaining caller is jmm_DumpHeap0(). You could use a stringStream to catch the output of HeapDumper::dump() and use that string to build the error message for the IOException in case of an error. I leave that up to you. --- Did you test that no jtreg tests fall over the changed output? e.g. test/jdk/sun/tools/jmap/BasicJMapTest.java, or whatever is under hotspot/jtreg/serviceability/jcmd ? Thanks, Thomas On Fri, Nov 8, 2019 at 12:56 PM Schmelter, Ralf wrote: > This change forwards the output from the HeapDumper.dump() command to an > optional output stream supplied by the caller. > Until now the diagnositic command and the "dumpheap" command of the attach > framework partly recreated the output by hand, but missing some information. > > Old output: > Heap dump file created > > New output: > Dumping heap to test.hprof ... > Heap dump file created [9719330384 bytes in 27,759 secs] > > In addition to getting this improved information, it saves code too. > > Bugreport: https://bugs.openjdk.java.net/browse/JDK-8233790 > Webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/ > > Best regards, > Ralf >
RFR (XS) 8233790: Forward output from heap dumper to jcmd/jmap
This change forwards the output from the HeapDumper.dump() command to an optional output stream supplied by the caller. Until now the diagnositic command and the "dumpheap" command of the attach framework partly recreated the output by hand, but missing some information. Old output: Heap dump file created New output: Dumping heap to test.hprof ... Heap dump file created [9719330384 bytes in 27,759 secs] In addition to getting this improved information, it saves code too. Bugreport: https://bugs.openjdk.java.net/browse/JDK-8233790 Webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8233790/webrev.0/ Best regards, Ralf
