templateInterpreter.cpp: copy_table() needs to be safer

This issue came up during the code review for the following fix:

    JDK-8227117 normal interpreter table is not restored after
                           single stepping with TLH

The issue is not caused by TLH or by JDK-8227117, but dates
back to the earliest days of HotSpot.

Here is Erik's code review comment and my reply:

On 7/5/19 1:07 PM, Daniel D. Daugherty wrote:
> On 7/4/19 3:10 AM, Erik Österlund wrote:
>> Hi Dan,
>>
>> Thanks for picking this up. The change looks good.
>
> Thanks! Of course, just the size of the comment below makes me wonder
> what I got myself into... :-) And I was so happy that the non-logging
> part of the fix was an else-statement with _one_ line...
>
>
>> 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.
>
> Actually, the comment doesn't mention 'atomic', but that's probably
> because the code and the comment are very, very old. It mentions
> 'word wise for MT safety' and I agree that 'atomic' is what the
> person likely meant...
>
> The history:
>
> $ sgv src/share/vm/interpreter/templateInterpreter.cpp | grep 'The copy has to occur word wise for MT safety'
> 1.1 // Copy non-overlapping tables. The copy has to occur word wise for MT safety.
>
> $ sp -r1.1 src/share/vm/interpreter/templateInterpreter.cpp
> src/share/vm/interpreter/SCCS/s.templateInterpreter.cpp:
>
> D 1.1 07/08/29 13:42:26 sgoldman 1 0 00600/00000/00000
> MRs:
> COMMENTS:
> 6571248 - continuation_for is specialized for template interpreter
>
> Hmmm... I expected that comment to be even older... ahhhh... a little
> more poking around and I found:
>
> $ sgv -r1.147 src/share/vm/interpreter/interpreter.cpp | grep 'The copy has to occur word wise for MT safety'
> 1.147 // Copy non-overlapping tables. The copy has to occur word wise for MT safety.
>
> $ sp -r1.147 src/share/vm/interpreter/interpreter.cpp
> src/share/vm/interpreter/SCCS/s.interpreter.cpp:
>
> D 1.147 99/02/17 10:14:36 steffen 235 233 00008/00002/00762
> MRs:
> COMMENTS:
>
> This makes more sense (timeline wise) and dates back to when all
> of the interpreter was in vm/interpreter/interpreter.cpp.
>
>
>> 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.
>
> Yet another C++ compiler optimization land mine... sigh...
>
>
>> 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.
>
> That last bit is scary...
>
>
>> 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.
>
> Copy::disjoint_words_atomic() sounds appealing...
>
> For those folks that aren't familiar with this part of safepointing...
>
> 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...
>
>
>> 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.
>
> Yes I would like to keep the copy_table() issue for a separate bug (not RFE).
> I'll file a follow up bug after the dust settles for 8227117.
>
> Thanks again for the review!
>
> Dan
>
>>
>> Thanks,
>> /Erik