Request for reviews (M): 7009756: volatile variables could be broken throw reflection API
David Holmes
David.Holmes at oracle.com
Wed Jan 5 18:02:36 PST 2011
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
>>>>>
More information about the hotspot-dev
mailing list