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

[Daniel D. Daugherty]

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

Copying part of my reply to Erik here:

SafepointSynchronize::arm_safepoint() calls Interpreter::notice_safepoints()
which calls calls copy_table(). So we're not at a safepoint yet, and, in 
fact,
we're trying to bring those pesky JavaThreads to a safepoint...

SafepointSynchronize::disarm_safepoint() calls 
Interpreter::ignore_safepoints()
which also calls copy_table(). However, we did that before we have woken the
JavaThreads that are blocked for the safepoint so that use of copy_table 
is safe:


   // Release threads lock, so threads can be created/destroyed again.
   Threads_lock->unlock();

   // Wake threads after local state is correctly set.
   _wait_barrier->disarm();
}

The 'Threads_lock->unlock()' should synchronize memory so that the restored
table should be properly synced out to memory...

Dan


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