changeset 11870:a8b9f9ed30ff

8157904: Atomic::cmpxchg for jbyte is missing a fence on initial failure Reviewed-by: simonis, aph, kbarrett
author dholmes
date Wed, 24 Aug 2016 19:54:03 -0400
parents 162138df46f7
children 750d4378c4a6 dd951011f433
files src/share/vm/runtime/atomic.hpp src/share/vm/utilities/globalDefinitions.hpp
diffstat 2 files changed, 36 insertions(+), 18 deletions(-) [+]
line wrap: on
line diff
--- a/src/share/vm/runtime/atomic.hpp	Wed Aug 24 20:38:21 2016 +0200
+++ b/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/src/share/vm/utilities/globalDefinitions.hpp	Wed Aug 24 20:38:21 2016 +0200
+++ b/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) {