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

[Daniel D. Daugherty]

Hi Serguei!

Thanks for the quick review. I didn't expect any U.S. reviewers to chime
in until after the 4th of July holiday...


On 7/3/19 11:34 PM, serguei.spitsyn at oracle.com wrote:
> Hi Dan,
>
> Thank you and Robbin for discovering and fixing this!
> The fix looks good to me.

Thanks!


> It is nice to have this new logging.

Yup. Will be very useful if we decide to write a test to verify
this isolated bit of expected behavior... :-)


> Should we fix this bug in 13 first, or you consider it risky?

No I don't consider it risky, but a number of things have come
up from the other reviewers so let's see where the dust settles...

Dan


>
> Thanks,
> Serguei
>
>
> On 7/3/19 7: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
>>
>