8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure
Reviewed-by: simonis, aph, kbarrett
--- a/hotspot/src/share/vm/runtime/atomic.hpp Wed Aug 24 20:38:21 2016 +0200
+++ b/hotspot/src/share/vm/runtime/atomic.hpp Wed Aug 24 19:54:03 2016 -0400
@@ -150,26 +150,39 @@
* as well as defining VM_HAS_SPECIALIZED_CMPXCHG_BYTE. This will cause the platform specific
* implementation to be used instead.
*/
-inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte *dest, jbyte comparand, cmpxchg_memory_order order)
-{
- assert(sizeof(jbyte) == 1, "assumption.");
- uintptr_t dest_addr = (uintptr_t)dest;
- uintptr_t offset = dest_addr % sizeof(jint);
- volatile jint* dest_int = (volatile jint*)(dest_addr - offset);
+inline jbyte Atomic::cmpxchg(jbyte exchange_value, volatile jbyte* dest,
+ jbyte compare_value, cmpxchg_memory_order order) {
+ STATIC_ASSERT(sizeof(jbyte) == 1);
+ volatile jint* dest_int =
+ static_cast<volatile jint*>(align_ptr_down(dest, sizeof(jint)));
+ size_t offset = pointer_delta(dest, dest_int, 1);
jint cur = *dest_int;
- jbyte* cur_as_bytes = (jbyte*)(&cur);
- jint new_val = cur;
- jbyte* new_val_as_bytes = (jbyte*)(&new_val);
- new_val_as_bytes[offset] = exchange_value;
- while (cur_as_bytes[offset] == comparand) {
- jint res = cmpxchg(new_val, dest_int, cur, order);
- if (res == cur) break;
+ jbyte* cur_as_bytes = reinterpret_cast<jbyte*>(&cur);
+
+ // current value may not be what we are looking for, so force it
+ // to that value so the initial cmpxchg will fail if it is different
+ cur_as_bytes[offset] = compare_value;
+
+ // always execute a real cmpxchg so that we get the required memory
+ // barriers even on initial failure
+ do {
+ // value to swap in matches current value ...
+ jint new_value = cur;
+ // ... except for the one jbyte we want to update
+ reinterpret_cast<jbyte*>(&new_value)[offset] = exchange_value;
+
+ jint res = cmpxchg(new_value, dest_int, cur, order);
+ if (res == cur) break; // success
+
+ // at least one jbyte in the jint changed value, so update
+ // our view of the current jint
cur = res;
- new_val = cur;
- new_val_as_bytes[offset] = exchange_value;
- }
+ // if our jbyte is still as cur we loop and try again
+ } while (cur_as_bytes[offset] == compare_value);
+
return cur_as_bytes[offset];
}
+
#endif // VM_HAS_SPECIALIZED_CMPXCHG_BYTE
inline unsigned Atomic::xchg(unsigned int exchange_value, volatile unsigned int* dest) {
--- a/hotspot/src/share/vm/utilities/globalDefinitions.hpp Wed Aug 24 20:38:21 2016 +0200
+++ b/hotspot/src/share/vm/utilities/globalDefinitions.hpp Wed Aug 24 19:54:03 2016 -0400
@@ -327,11 +327,12 @@
// and then additions like
// ... top() + size ...
// are safe because we know that top() is at least size below end().
-inline size_t pointer_delta(const void* left,
- const void* right,
+inline size_t pointer_delta(const volatile void* left,
+ const volatile void* right,
size_t element_size) {
return (((uintptr_t) left) - ((uintptr_t) right)) / element_size;
}
+
// A version specialized for HeapWord*'s.
inline size_t pointer_delta(const HeapWord* left, const HeapWord* right) {
return pointer_delta(left, right, sizeof(HeapWord));
@@ -516,6 +517,10 @@
return (void*)align_size_down((intptr_t)ptr, (intptr_t)alignment);
}
+inline volatile void* align_ptr_down(volatile void* ptr, size_t alignment) {
+ return (volatile void*)align_size_down((intptr_t)ptr, (intptr_t)alignment);
+}
+
// Align metaspace objects by rounding up to natural word boundary
inline intptr_t align_metadata_size(intptr_t size) {