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

[Daniel D. Daugherty]

Thanks for the re-review!

Dan


On 7/7/19 5:16 AM, serguei.spitsyn at oracle.com wrote:
> Hi Dan,
>
> The update looks good to me.
>
> Thanks,
> Serguei
>
>
> On 7/5/19 13:24, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> Here's the new webrev URLs after CR0:
>>
>> Full:
>>
>> http://cr.openjdk.java.net/~dcubed/8227117-webrev/1_for_jdk14.full/
>>
>> Inc:
>>
>> http://cr.openjdk.java.net/~dcubed/8227117-webrev/1_for_jdk14.inc/
>>
>> Only src/hotspot/share/prims/jvmtiEventController.cpp changed in this
>> round. The only code change was this:
>>
>>  VM_ChangeSingleStep::VM_ChangeSingleStep(bool on)
>> -  : _on(on != 0)
>> +  : _on(on)
>>  {
>>  }
>>
>> The other change is a header comment block rewrite...
>>
>> Thanks, in advance, for questions, comments or suggestions.
>>
>> Dan
>>
>>
>>
>> On 7/3/19 10: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.
>>>
>>> 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();
>>>    }
>>>
>>> Everything else is just new logging support for future debugging of
>>> interpreter table management and single stepping.
>>>
>>> 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
>>>
>>>
>>
>
>