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

[John Rose]

On Jan 3, 2011, at 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.

The ObjectLocker-based implementation of GetVolatileLong will fail on a null base pointer, but that is allowed by the Unsafe API.  This is another bug which Vladimir's change removes.

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


If an Unsafe.*Volatile* method does not correctly emulate Java volatile field semantics, that is a bug.

(One reservation:  I assume there is no code in JSR 166 stuff that relies for performance on weakened non-atomic semantics on longs.)

I agree the fence structure for volatiles should be the same in unsafe.cpp as it is in the interpreter and compiler.

-- John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20110104/c6f1878c/attachment.html