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

[David Holmes]

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.

  > 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).

> 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'.

> 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