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

[coleen.phillimore at oracle.com]

On 7/8/19 11:42 AM, Daniel D. Daugherty wrote:
> 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...

Sometimes Reply-All in my mailer gets all the lists and sometimes it 
doesn't.  I'll check next time that I get them all.
>
> 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...

Thanks, I didn't mind not being listed but I was interested in the 
change and how it escaped our testing.

Coleen

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