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

[Vladimir Kozlov]

David,

What mechanism you are talking about?:
"Such a mechanism already exists for regular volatile variable support."
As you may notice I duplicated code from these macros.

supports_cx8 and lock are not needed here if you have atomic long move instruction.

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.

I think this code was duplicated without thinking from Unsafe_CompareAndSwapLong()
when it was fixed long time ago to support HW without cx8.

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