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

[Andrew Dinn]

The following patch corrects the use of barriers in the JavaFrameAnchor
routines in JDK8. It follows the rationale provided in the corresponding
push to the icedtea7-forest-aarch64 repo (see attached note for details).

Is it ok to push this change? or do I need to wait for it to go into
JDK9 first?

n.b. I have checked the latest JDK9 code and believe the same patch will
apply correctly to JDK9 and be valid for the same reasons.

regards,


Andrew Dinn
-----------
# HG changeset patch
# User adinn
# Date 1416913376 0
#      Tue Nov 25 11:02:56 2014 +0000
# Node ID a10d557dd569efcae705a0c692b407dcd2fff614
# Parent  6fb37d6acb123dc55d754b4fdc9bbb9957ac61ac
correct calls to OrderAccess::release when updating java anchors

diff -r 6fb37d6acb12 -r a10d557dd569
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 11:02:56
2014 +0000
@@ -48,20 +48,16 @@
   }

   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. it is safe to modify fp and pc when copying into the
+    // thread anchor. sp will already have been NULLed by zap() and
+    // the write published by a fence in the state transition to in_vm
+    // copying into the wrapper anchor has no implications since that
+    // object is thread local until the subsequent entry into java
     _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 +76,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

-------------- next part --------------
An embedded message was scrubbed...
From: Andrew Dinn <adinn at redhat.com>
Subject: Re: [aarch64-port-dev ] Query regarding backport of memory order changes to JDK7
Date: Thu, 20 Nov 2014 11:09:57 +0000
Size: 11578
URL: <http://mail.openjdk.java.net/pipermail/aarch64-port-dev/attachments/20141125/e9f35cf8/AttachedMessage.mht>