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

[David Holmes]

David Holmes said the following on 01/04/11 13:01:
> Further, note that the specification for Unsafe does not make it clear 
> that these methods are supposed to implement the atomicity properties of 
> Java volatiles, as opposed to simply adhering to the memory model 
> properties.

Ignore the above - the usage by AtomicLongFieldUpdater makes it clear 
that 64-bit atomic access is expected/required.

David

> 
> David
> 
> David Holmes said the following on 01/04/11 12:48:
>> 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