RFR(XXS): 8227117: normal interpreter table is not restored after single stepping with TLH
David Holmes
david.holmes at oracle.com
Thu Jul 4 07:18:45 UTC 2019
PS. I just noticed this comment:
// This change must always be occur when at a safepoint.
// Being at a safepoint causes the interpreter to use the
// safepoint dispatch table which we overload to find single
// step points. Just to be sure that it has been set, we
// call notice_safepoints when turning on single stepping.
// When we leave our current safepoint, should_post_single_step
// will be checked by the interpreter, and the table kept
// or changed accordingly.
void VM_ChangeSingleStep::doit() {
The "when we leave the safepoint" part is actually the bug that is being
fixed - right? So the comment is not accurate.
David
-----
On 4/07/2019 5:13 pm, 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?
>
>> 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)
> {
> }
>
> 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,
> 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
>>
More information about the serviceability-dev
mailing list