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

[Vladimir Kozlov]

Thank you, David

Vladimir

On 1/5/11 6:02 PM, David Holmes wrote:
> Vladimir Kozlov said the following on 01/06/11 11:11:
>> After long internal discussion I decided to keep old code
>> in unsafe.cpp undef #ifdef for platforms other than Sparc and X86.
>>
>> http://cr.openjdk.java.net/~kvn/7009756/webrev.02
>
> That's certainly not the cleanest way to implement things. Much of the ifdef logic is superfluous given that
> supports_cx8 will be true for x86 and sparc anyway. If we switch to using supports_atomic_XXXX in the future then we
> will rip most of this back out.
>
> But I won't stand in the way.
>
> David
>
>> Thanks,
>> Vladimir
>>
>>
>> Vladimir Kozlov wrote:
>>> 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
>>>>>>