[aarch64-port-dev ] Correct the placement of release calls in JDK8 JavaFrameAnchor code

[Andrew Dinn]

On 25/11/14 15:00, Andrew Haley wrote:
> On 11/25/2014 02:51 PM, Andrew Dinn wrote:
>> I'm happy to wait if need be.
> 
> Ah, okay.  yes, please wait for the push to 9.
> 
>> Yes, that is what I mean. Do you want me to upgrade the level of detail
>> in the comment?
> 
> Please.

Corrected patch.

regards,


Andrew Dinn
-----------
# HG changeset patch
# User adinn
# Date 1416928588 0
#      Tue Nov 25 15:16:28 2014 +0000
# Node ID d44e30f7a3435acdc42aa8ed852c280621c2596f
# Parent  6fb37d6acb123dc55d754b4fdc9bbb9957ac61ac
correct calls to OrderAccess::release when updating java anchors

diff -r 6fb37d6acb12 -r d44e30f7a343
src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp
--- a/src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp	Tue Nov 18 14:20:24
2014 +0000
+++ b/src/cpu/aarch64/vm/javaFrameAnchor_aarch64.hpp	Tue Nov 25 15:16:28
2014 +0000
@@ -48,20 +48,22 @@
   }

   void copy(JavaFrameAnchor* src) {
-    // In order to make sure the transition state is valid for "this"
-    // We must clear _last_Java_sp before copying the rest of the new data
-    //
-    // Hack Alert: Temporary bugfix for 4717480/4721647
-    // To act like previous version (pd_cache_state) don't NULL
_last_Java_sp
-    // unless the value is changing
-    //
-    if (_last_Java_sp != src->_last_Java_sp) {
-      _last_Java_sp = NULL;
-      OrderAccess::release();
-    }
+    // n.b. the writes to fp and pc do not require any preceding
+    // release(). when copying into the thread anchor, which only
+    // happens under ~JavaCallWrapper(), sp will have been NULLed by a
+    // call to zap() and the NULL write will have been published by a
+    // fence in the state transition to in_vm. contrariwise, when
+    // copying into the wrapper anchor, which only happens under
+    // JavaCallWrapper(), there is no ordering requirement at all
+    // since that object is thread local until the subsequent entry
+    // into java. JavaCallWrapper() call clear() after copy() thus
+    // ensuring that all 3 writes are visible() before the wrapper is
+    // accessible to other threads.
     _last_Java_fp = src->_last_Java_fp;
     _last_Java_pc = src->_last_Java_pc;
-    // Must be last so profiler will always see valid frame if
has_last_frame() is true
+    // Must be last so profiler will always see valid frame if
+    // has_last_frame() is true
+    OrderAccess::release();
     _last_Java_sp = src->_last_Java_sp;
   }

@@ -80,10 +82,14 @@

 public:

-  void set_last_Java_sp(intptr_t* sp)            { _last_Java_sp = sp;
OrderAccess::release(); }
+  // n.b. set_last_Java_sp and set_last_Java_fp are never called
+  // (which is good because they would need a preceding or following
+  // call to OrderAccess::release() to make sure the writes are
+  // visible in the correct order).
+void set_last_Java_sp(intptr_t* sp)            { assert(false, "should
not be called"); _last_Java_sp = sp; }

   intptr_t*   last_Java_fp(void)                     { return
_last_Java_fp; }
   // Assert (last_Java_sp == NULL || fp == NULL)
-  void set_last_Java_fp(intptr_t* fp)                {
OrderAccess::release(); _last_Java_fp = fp; }
+  void set_last_Java_fp(intptr_t* fp)                { assert(false,
"should not be called"); _last_Java_fp = fp; }

 #endif // CPU_AARCH64_VM_JAVAFRAMEANCHOR_AARCH64_HPP