Request for reviews (M): 7009756: volatile variables could be broken throw reflection API
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Jan 5 17:11:32 PST 2011
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
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
>>>>
More information about the hotspot-dev
mailing list