C1 code installation and JNIHandle::deleted_handle() oop

[Vladimir Ivanov]

FYI just filed JDK-8191227 [1]. Thanks for the report!

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8191227

On 11/14/17 9:18 PM, Aleksey Shipilev wrote:
> 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
>