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

[Daniel D. Daugherty]

On 7/8/19 11:30 AM, coleen.phillimore at 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...

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