RFR(XXS): 8227117: normal interpreter table is not restored after single stepping with TLH

[Daniel D. Daugherty]

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.


>
>> 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
>>