RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

[Daniel D. Daugherty]

Hi Kim,

Thanks for the review.


On 7/8/19 7:08 PM, Kim Barrett wrote:
>> On Jul 6, 2019, at 9:53 AM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>>
>> Greetings,
>>
>> During the code review for the following fix:
>>
>>      JDK-8227117 normal interpreter table is not restored after single stepping with TLH
>>      https://bugs.openjdk.java.net/browse/JDK-8227117
>>
>> Erik O. noticed a potential race with templateInterpreter.cpp: copy_table()
>> depending on C++ compiler optimizations. The following bug is being used
>> to fix this issue:
>>
>>      JDK-8227338 templateInterpreter.cpp: copy_table() needs to be safer
>>      https://bugs.openjdk.java.net/browse/JDK-8227338
>>
>> Here's the webrev URL:
>>
>>      http://cr.openjdk.java.net/~dcubed/8227338-webrev/0_for_jdk14/
>>
>> This fix has been tested via Mach5 Tier[1-3] on Oracle's usual platforms.
>> Mach5 tier[4-6] is running now. It has also been tested with the manual
>> jdb test from JDK-8227117 using 'release' and 'fastdebug' bits.
>>
>> Thanks, in advance, for questions, comments or suggestions.
>>
>> Dan
> [This review is ignoring the issues around the current implementation
> of atomic copies discussed elsewhere in this thread. I assume those
> will be addressed elsewhere.]
>
> ------------------------------------------------------------------------------
> src/hotspot/share/interpreter/templateInterpreter.cpp
>   286     while (size-- > 0) *to++ = *from++;
>
> [pre-existing]
>
> This ought to be using Copy::disjoint_words.  That's even more obvious
> in conjunction with the change to use Copy::disjoint_words_atomic in
> the non-safepoint case.

I can make that change. Is there a specific advantage/reason that you
have in mind here?


> ------------------------------------------------------------------------------
> src/hotspot/share/interpreter/templateInterpreter.cpp
>   284   if (SafepointSynchronize::is_at_safepoint()) {
>
> I wonder how much benefit we really get from having distinct safepoint
> and non-safepoint cases, rather than just unconditionally using
> Copy::disjoint_words_atomic.

Sorry, I don't know the answer to that. My intention was to use
Copy::disjoint_words_atomic() only in the case where I knew that
I needed it so no potential impact on existing uses at a safepoint.

Thanks for the review.

Dan


>
> ------------------------------------------------------------------------------
>