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

[David Holmes]

Hi Erik,

On 4/07/2019 5:10 pm, Erik Österlund wrote:
> Hi Dan,
> 
> Thanks for picking this up. The change looks good.
> 
> However, when reviewing this, I looked at the code for actually 
> restoring the table (ignore/notice safepoints). It copies the dispatch 
> table for the interpreter. There is a comment stating it is important 
> the copying is atomic for MT-safety, and I can definitely see why. 
> However, the copying the line after that comment is in fact not atomic.

Is it assuming "atomicity" by virtue of executing at a safepoint?

David
-----

> Here is the copying code in templateInterpreter.cpp:
> 
> static inline void copy_table(address* from, address* to, int size) {
>    // Copy non-overlapping tables. The copy has to occur word wise for 
> MT safety.
>    while (size-- > 0) *to++ = *from++;
> }
> 
> Copying using a loop of non-volatile loads and stores can and definitely 
> will on some compilers turn into memcpy calls instead as the compiler 
> (correctly) considers that an equivalent transformation. And memcpy does 
> not guarantee atomicity. Indeed on some platforms it is not atomic. On 
> some platforms it will even enjoy out-of-thin-air values. Perhaps 
> Copy::disjoint_words_atomic() would be a better choice for atomic word 
> copying. If not, at the very least we should use Atomic::load/store here.
> 
> Having said that, the fix for that issue seems like a separate RFE, 
> because it has been sitting there for a lot longer than TLH has been 
> around.
> 
> Thanks,
> /Erik
> 
> On 2019-07-04 04:04, 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
>>