Added back serviceability-dev@...
Coleen, please double check before using reply-to-list... if there's more
than one list, that feature doesn't work right...
On 7/8/19 11:34 AM, Daniel D. Daugherty wrote:
On 7/8/19 11:30 AM, coleen.phillim...@oracle.com wrote:
The change and comment look good. I have a question below though:
Thanks for the review. I've already committed the changeset
in preparation for pushing. Hope you don't mind if I don't
list you as a reviewer...
I was able to use "hg rollback" and redo the patch to include
you in the list of reviewers...
Dan
More below...
On 7/5/19 1:12 PM, Daniel D. Daugherty wrote:
On 7/4/19 3:13 AM, David Holmes wrote:
Hi Dan,
On 4/07/2019 12:04 pm, Daniel D. Daugherty wrote:
Greetings,
Robbin recently discovered this issue with Thread Local
Handshakes. Since
he's not available at the moment, I'm handling the issue:
JDK-8227117 normal interpreter table is not restored after
single stepping with TLH
https://bugs.openjdk.java.net/browse/JDK-8227117
When using Thread Local Handshakes, the normal interpreter table is
not restored after single stepping. This issue is caused by the
VM_ChangeSingleStep VM-op relying on SafepointSynchronize::end() to
restore the normal interpreter table for the "off" case.
So the result of this is that debugging tests may run more slowly
overall?
Not just tests. An interactive debugging session would also be
affected.
After single stepping once, we won't ever switch back to the normal
table
so we'll be stuck with the safepoint interpreter dispatch table.
I'm trying to think if there's a good assertion to test this, but I
don't think there is. Maybe renaming the safept_table to
breakpoint_table would be good to do to make it clear, once TLH is
the only way to safepoint.
Definitely something to keep in mind for the future. I don't
know if there is a bug for eventually phasing out global
safepoints...
Dan
Thanks,
Coleen
Prior to Thread Local Handshakes, this was a valid assumption to
make.
SafepointSynchronize::end() has been refactored into
disarm_safepoint() and it only calls Interpreter::ignore_safepoints()
on the global safepoint branch. That matches up with the call to
Interpreter::notice_safepoints() that is also on the global safepoint
branch.
The solution is for the VM_ChangeSingleStep VM-op for the "off" case
to call Interpreter::ignore_safepoints() directly.
Here's the webrev URL:
http://cr.openjdk.java.net/~dcubed/8227117-webrev/0_for_jdk14/
The fix is just a small addition to VM_ChangeSingleStep::doit():
if (_on) {
Interpreter::notice_safepoints();
+ } else {
+ Interpreter::ignore_safepoints();
}
Looks good - thanks for the detailed analysis in the bug report.
I have on additional request from looking at related code - can you
fix this confused initializer:
VM_ChangeSingleStep::VM_ChangeSingleStep(bool on)
: _on(on != 0)
{
}
Yes, I can fix that.
as _on and on are both bool the assignment can be direct and we
shouldn't be comparing a bool to 0 as a matter of style. Thanks.
Everything else is just new logging support for future debugging of
interpreter table management and single stepping.
Logging looks good too.
Thanks.
Thanks for the review.
Dan
Thanks,
David
-----
Tested this fix with Mach5 Tier[1-3] on the standard Oracle
platforms.
Mach5 Tier[4-6] on standard Oracle platforms is running now.
Thanks, in advance, for questions, comments or suggestions.
Dan
