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

[Daniel D. Daugherty]

On 7/4/19 3:18 AM, David Holmes wrote:
> 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.

I'll take a closer look at this part of the comment:

// When we leave our current safepoint, should_post_single_step
// will be checked by the interpreter, and the table kept
// or changed accordingly.

and figure out how to clarify it as part of this change.

Dan


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