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

[Daniel D. Daugherty]

Added back serviceability-dev at ...

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

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