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

Tue Jan 4 14:29:40 PST 2011

David,

We talked on today compiler meeting about this bug and
decided that I will do only small additional changes:
add memory barriers to Unsafe (C2 has barriers) and
use Atomic:: moves for long in OrderAccess methods.

I will leave support for other platforms to
runtime and embedded groups. But keep in mind that
ObjectLocker-based implementation will fail as
John Rose pointed. And it would be nice to keep
platform specific code inside Atomic:: methods.

Thanks,
Vladimir

David Holmes wrote:
>  > 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
>>>