C1 code installation and JNIHandle::deleted_handle() oop

[Aleksey Shipilev]

On 11/14/2017 05:37 PM, Aleksey Shipilev wrote:
> On 11/14/2017 05:23 PM, Vladimir Ivanov wrote:
>>> Actually, in Shenandoah case, the failure is here (it is obscured by inlining, but visible in
>>> slowdebug):
>>>    http://hg.openjdk.java.net/jdk/hs/file/5caa1d5f74c1/src/hotspot/share/runtime/jniHandles.hpp#l220
>>>
>>> ... "== deleted_handle()" or "!= deleted_handle()". So fortifying assert code does not help, the bug
>>> is touching the oop in improper thread state. Nightmarish scenario would be to fail "!="/"==" check
>>> because the deleted_handle oop had been already updated, but the actual handle value did not.
>>
> 
> See this line:
>   http://hg.openjdk.java.net/jdk/hs/file/5caa1d5f74c1/src/hotspot/share/runtime/jniHandles.hpp#l224
> 
>   } else if ((value == badJNIHandle) || (value == deleted_handle())) {
>     value = NULL;
>   }
>   return value;
> 
> If we ever get in that code during the GC, we might get the deleted_handle() updated by GC code
> (because it is a root via JNIHandles), but the $value may be not updated, and still point to old
> location. Then "value == deleted_handle()" fails, and we return broken oop from guard_value. Notice
> this code is not under the assert.
> 
> It seems to me the symptom of larger problem: touching naked oops is seldom reliable, if the thread
> is inconsistent state. It does not seem right to fix asserts, because the real trouble is touching
> that oop on product paths. It appears to work for other GCs, but for Shenandoah this failure is
> observable, because its barriers touch heap through the oops.

FTR, this is the minimal change that makes Shenandoah pass the stress tests:

$ hg diff
diff -r ac4d369eac91 src/hotspot/share/code/debugInfo.cpp
--- a/src/hotspot/share/code/debugInfo.cpp	Tue Nov 14 12:27:52 2017 +0100
+++ b/src/hotspot/share/code/debugInfo.cpp	Tue Nov 14 17:54:44 2017 +0100
@@ -209,6 +209,7 @@
 // ConstantOopWriteValue

 void ConstantOopWriteValue::write_on(DebugInfoWriteStream* stream) {
+  VM_ENTRY_MARK;
   assert(JNIHandles::resolve(value()) == NULL ||
          Universe::heap()->is_in_reserved(JNIHandles::resolve(value())),
          "Should be in heap");

And this is the change that avoids the Shenandoah crash (because it does not do read barriers on
value and deleted_handle()), but still is subject to GC-compiler race explained above:

$ hg diff
diff -r ac4d369eac91 src/hotspot/share/runtime/jniHandles.hpp
--- a/src/hotspot/share/runtime/jniHandles.hpp	Tue Nov 14 12:27:52 2017 +0100
+++ b/src/hotspot/share/runtime/jniHandles.hpp	Tue Nov 14 17:58:22 2017 +0100
@@ -219,8 +219,8 @@
 inline oop JNIHandles::guard_value(oop value) {
   if (!external_guard) {
     assert(!oopDesc::unsafe_equals(value, badJNIHandle), "Pointing to zapped jni handle area");
-    assert(!oopDesc::equals(value, deleted_handle()),    "Used a deleted global handle");
-  } else if (oopDesc::unsafe_equals(value, badJNIHandle) || oopDesc::equals(value, deleted_handle())) {
+    assert(!oopDesc::unsafe_equals(value, deleted_handle()), "Used a deleted global handle");
+  } else if (oopDesc::unsafe_equals(value, badJNIHandle) || oopDesc::unsafe_equals(value,
deleted_handle())) {
     value = NULL;
   }
   return value;

Neither feels like a proper fix.

Thanks,
-Aleksey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20171114/a0e3a6c9/signature-0001.asc>