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

[Daniel D. Daugherty]

On 7/9/19 9:13 AM, Daniel D. Daugherty wrote:
> Hi Kim,
>
> Thanks for the review.
>
>
> On 7/8/19 7:00 PM, Kim Barrett wrote:
>>> On Jul 7, 2019, at 8:08 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> On 7/07/2019 6:48 pm, Erik Osterlund wrote:
>>>> The real danger is SPARC though and its BIS instructions. I don’t 
>>>> have the code in front of me, but I really hope not to see that 
>>>> switch statement and non-volatile loop in that 
>>>> pd_disjoint_words_atomic() function.
>>> sparc uses the same loop.
>>>
>>> Let's face it, almost no body expects the compiler to do these kinds 
>>> of transformations. :(
>> See JDK-8131330 and JDK-8142368, where we saw exactly this sort of 
>> transformation from a fill-loop
>> to memset (which may use BIS, and indeed empirically does in some 
>> cases).  The loops in question
>> seem trivially convertible to memcpy/memmove.
>
> Very interesting reads. Thanks for pointing those out.
>
> src/hotspot/share/interpreter/templateInterpreter.cpp:
>
> DispatchTable TemplateInterpreter::_active_table;
> DispatchTable TemplateInterpreter::_normal_table;
> DispatchTable TemplateInterpreter::_safept_table;
>
> So it seems like changing _active_table to:
>
> volatile DispatchTable TemplateInterpreter::_active_table;
>
> might be a good idea... Do you concur?

This change would require a bunch of additional changes so I'm
not planning to make it (way too intrusive).

Dan


>
>
>> Also see JDK-8142349.
>>
>>>> And I agree that the atomic copying API should be used when we need 
>>>> atomic copying. And if it turns out the implementation of that API 
>>>> is not atomic, it should be fixed in that atomic copying API.
>>> I agree to some extent, but we assume atomic load/stores of words 
>>> all over the place - and rightly so. The issue here is that we need 
>>> to hide the loop inside an API that we can somehow prevent the C++ 
>>> compiler from screwing up. It's hardly intuitive or obvious when 
>>> this is needed e.g if I simply copy three adjacent words without a 
>>> loop could the compiler convert that to a block move that is 
>>> non-atomic?
>>>
>>>> So I think this change looks good. But it looks like we are not 
>>>> done yet. :c
>>> I agree that changing the current code to use the atomic copy API to 
>>> convey intent is fine.
>> I’ve been reserving Atomic::load/store for cases where the location 
>> “ought” to be declared std::atomic<T> if
>> we were using C++11 atomics (or alternatively some homebrew 
>> equivalent).  Not all places where we do
>> stuff “atomically” is appropriate for that though (consider card 
>> tables, being arrays of bytes, where using an
>> atomic<T> type might impose alignment constraints that are 
>> undesirable here).  I *think* just using volatile
>> here would likely be sufficient, e.g. we should have
>>
>>      Copy::disjoint_words_atomic(const HeapWord* from,volatile 
>> HeapWord* to, size_t count)
>
> I think this part should be taken up in the follow bug that I filed:
>
>     JDK-8227369 pd_disjoint_words_atomic() needs to be atomic
>     https://bugs.openjdk.java.net/browse/JDK-8227369
>
> Thanks for chiming in on the review!
>
> Dan
>
>