Request for reviews (M): 7009756: volatile variables could be broken throw reflection API

[David Holmes]

 > You are right that we should have code for platforms
 > which do not have 64-bit atomic moves but we can't
 > use supports_cx8. I will add a different proxy.

Thanks I'm fine with that.

And yes we need to fix intepreter too. C1 is okay (for atomicity and 
barriers), can't comment on C2.

As noted in other email though the Unsafe operations are missing all the 
needed memory barriers for volatile accesses, as are the intrinsic 
version in C1 (haven't checked C2).

So there is a lot of work to be done to get correct volatile support in 
place, and to fix up Unsafe volatile support.

Thanks,
David

Vladimir Kozlov said the following on 01/05/11 06:04:
> Tom Rodriguez wrote:
>> On Jan 4, 2011, at 3:46 AM, David Holmes wrote:
>>
>>> Vladimir Kozlov said the following on 01/04/11 14:19:
>>>> David,
>>>> What mechanism you are talking about?:
>>>> "Such a mechanism already exists for regular volatile variable 
>>>> support."
>>> I mean that we have code in the VM that ensures that load/store of 
>>> volatile Java variables is atomic. Those Unsafe macros should have 
>>> utilized similar code, not plain load/store.
>>>
> 
> There are NO uniform code for volatile moves.
> 
> 
> Interpreter:
> 
>  obj->long_field_acquire(field_offset), 0);
> 
>  obj->release_long_field_put(field_offset, STACK_LONG(-1));
>  OrderAccess::storeload();
> 
> Note: they are also NOT atomic and need to be fixed!!!
> 
> They call OrderAccess::load_acquire() and OrderAccess::release_store()
> which are implemented on x86 as:
> 
> inline jlong    OrderAccess::load_acquire(volatile jlong*   p) { return 
> *p; }
> inline void     OrderAccess::release_store(volatile jlong*   p, jlong   
> v) { *p = v; }
> 
> 
> Template interpreter uses FPU registers:
> 
>   __ push(rdx);
>   __ push(rax);                 // Must update atomically with FIST
>   __ fild_d(Address(rsp,0));    // So load into FPU register
>   __ fistp_d(lo);               // and put into memory atomically
>   __ addptr(rsp, 2*wordSize);
>   // volatile_barrier();
>   volatile_barrier(Assembler::Membar_mask_bits(Assembler::StoreLoad |
>                                                Assembler::StoreStore));
> 
>   __ fild_d(lo);                // Must load atomically
>   __ subptr(rsp,2*wordSize);    // Make space for store
>   __ fistp_d(Address(rsp,0));
>   __ pop(rax);
>   __ pop(rdx);
> 
> 
> C1 and C2 generated code uses FPU or XMM registers to move volatile long 
> atomically.
> C2 does generate memory barriers for volatile loads and stores.
> I am not familiar with C1 to say if it does.
> 
> 
>>>> supports_cx8 and lock are not needed here if you have atomic long move
>>>> instruction.
>>> _if_ you have them. That's what supports_cx8 is telling you. (Yes I 
>>> know it's an abuse but that's the way it is).
> 
> You are right that we should have code for platforms
> which do not have 64-bit atomic moves but we can't
> use supports_cx8. I will add a different proxy.
> I also realized that I did not add Atomic::load to
> zero implementation which will prevent its build.
> 
> Vladimir
> 
>>>
>>>> supports_cx8 is NOT indicating "whether atomic 64-bit operations are 
>>>> supported",
>>>> it only indicates that HW has 8 bytes atomic CMPXCHG instruction 
>>>> which is not used here.
>>> Please read the comment at 259-260 that you indicated you would 
>>> "clean up"
>>>
>>> 259 // Volatile long versions must use locks if 
>>> !VM_Version::supports_cx8().
>>> 260 // support_cx8 is a surrogate for 'supports atomic long memory ops'.
>>
>> That's the point of the fix, supports_cx8 is a bad proxy for whether 
>> you have atomic long operations.  32 bit x86 doesn't have an atomic 
>> long move but it does have cx8.  If other architectures need to use 
>> the ObjectLocker for atomic longs then we need some new feature test 
>> to drive that logic.  How are the compiler intrinsics for those 
>> routines implemented for those architectures?
>>
>> tom
>>
>>>> I think this code was duplicated without thinking from 
>>>> Unsafe_CompareAndSwapLong()
>>>> when it was fixed long time ago to support HW without cx8.
>>> I think it stems from there being platforms without the ability to do 
>>> 64-bit atomic load/stores.
>>>
>>> David
>>>
>>>> Thanks,
>>>> Vladimir
>>>> On 1/3/11 6:48 PM, David Holmes wrote:
>>>>> Vladimir Kozlov said the following on 01/04/11 12:28:
>>>>>> http://cr.openjdk.java.net/~kvn/7009756/webrev
>>>>>>
>>>>>> Fixed 7009756: volatile variables could be broken throw reflection 
>>>>>> API
>>>>>>
>>>>>> Unsafe_SetLongVolatile() and Unsafe_GetLongVolatile() are not atomic
>>>>>> on 32 bit x86 (C++ generates two 32 bit loads/stores).
>>>>>>
>>>>>> Use Atomic::load() and Atomic::store() to access a volatile long.
>>>>>>
>>>>>> Verified with bug's test on all 32 bit platforms. Passed JPRT.
>>>>> Hold up there! The Unsafe code uses supports_cx8 to determine 
>>>>> whether atomic 64-bit operations are supported, and if not
>>>>> it falls back to using explicit locking. This change totally 
>>>>> removes that for these methods and so breaks all platforms
>>>>> where cx8 is not supported.
>>>>>
>>>>> If anything is wrong it is these macros:
>>>>>
>>>>> 155 #define GET_FIELD_VOLATILE(obj, offset, type_name, v) \
>>>>> 156 oop p = JNIHandles::resolve(obj); \
>>>>> 157 volatile type_name v = *(volatile 
>>>>> type_name*)index_oop_from_field_offset_long(p, offset)
>>>>> 158
>>>>> 159 #define SET_FIELD_VOLATILE(obj, offset, type_name, x) \
>>>>> 160 oop p = JNIHandles::resolve(obj); \
>>>>> 161 *(volatile type_name*)index_oop_from_field_offset_long(p, 
>>>>> offset) = x; \
>>>>> 162 OrderAccess::fence();
>>>>> 163
>>>>>
>>>>> because we should be using a load/store mechanism that is aware of 
>>>>> the volatile aspect and so enforces the 64-bit
>>>>> atomicity requirement at a lower level. Such a mechanism already 
>>>>> exists for regular volatile variable support.
>>>>>
>>>>> David
>>