C1 code installation and JNIHandle::deleted_handle() oop

[Aleksey Shipilev]

On 11/14/2017 06:04 PM, Aleksey Shipilev wrote:
> 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");
> 
> Neither feels like a proper fix.

Well, VM_ENTRY_MARK is a decent enough workaround, so we are flying with that:
  http://mail.openjdk.java.net/pipermail/shenandoah-dev/2017-November/004296.html

Happy to pick up the proper fix, if this is not the one.

-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/d7c5e4f7/signature.asc>