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

[Daniel D. Daugherty]

On 7/4/19 5:38 AM, David Holmes wrote:
> Hi Erik,
>
> On 4/07/2019 6:08 pm, Erik Österlund wrote:
>> Hi David,
>>
>> When you run without TLH, this copying mechanism is used to 
>> synchronize the safepoint while JavaThreads are running. The 
>> interpreter doesn't emit any polls then. Instead it clobbers the 
>> dispatch table. JavaThreads will be reading from the dispatch table 
>> while it is being (non-atomically) modified. That could crash. For 
>> example with the Solaris + studio + SPARC - TLH configuration, the 
>> compiler will almost certainly emit a memcpy (this transformation has 
>> been observed in practice), the memcpy will use BIS instructions 
>> (observed in practice) for performance, with out-of-thin-air values 
>> (observed in practice), and the JavaThreads will occasionally crash 
>> during safepoint synchronization due to said out-of-thin-air values.
>>
>> So I guess the problem might be larger back when TLH was not default. 
>> But this seems conceptually wrong.
>
> I always thought there were two dispatch tables and we simply switched 
> between them - not copied anything!

Based on my search back into the TeamWare repos, it looks like we have
always copied the table...

Dan


>
> David
>
>> /Erik
>>
>> On 2019-07-04 09:17, 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?
>>>
>>> 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
>>>>>
>>