RFR (S) [Graal] runtime/CommandLine/PrintTouchedMethods.java crashes with assertion "reference count underflow for symbol"

[David Holmes]

On 26/03/2018 1:23 AM, coleen.phillimore at oracle.com wrote:
> On 3/25/18 9:08 AM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 24/03/2018 7:38 AM, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8199793
>>> http://cr.openjdk.java.net/~iklam/jdk11/8199793-PrintTouchedMethods-crash.v01/ 
>>>
>>>
>>>
>>> ANALYSIS:
>>>
>>> The crash is in
>>>
>>>      V [libjvm.so+0x16fe226] Symbol::decrement_refcount()+0xe6
>>>      V [libjvm.so+0x1026e0b] JVM_FindLoadedClass+0x20b
>>>
>>> and the log file says "Symbol: 'java/lang/invoke/LambdaForm$BMH' 
>>> count -1".
>>>
>>> This seems to be a race condition between Symbol::decrement_refcount()
>>> vs Symbol::set_permanent(). The former uses an atomic increment and 
>>> is called by
>>> JVM_FindLoadedClass. The latter does a simple store of a short value 
>>> of -1, and is
>>> called only by Method::log_touched() when -XX:+LogTouchedMethods is 
>>> enabled.
>>>
>>> Apparently we have a Symbol whose refcount started with a positive 
>>> value.
>>> While one thread is calling Symbol::decrement_refcount() and a second
>>> thread calls Symbol::set_permanent() at the same time, the unexpected 
>>> value -1
>>> could be returned to the first thread.
>>>
>>> FIX:
>>>
>>> I changed Method::log_touched() to use Symbol::increment_refcount 
>>> instead.
>>> I can no longer reproduce the crash after this change.
>>>
>>> Also, because the behavior of Symbol::set_permanent is not well 
>>> understood
>>> and has shown to be racy, I removed this function altogether.
>>
>> So when will a Symbol now ever be "permanent"? Isn't all the code 
>> related to is_permanent() and PERM_REFCOUNT dead now? And the checks 
>> for _refcount >= 0 ?
> 
> When we create a symbol, it can have a permanent refcount.

Okay so now there are two types of "permanent" symbols - which seems 
confusing. Those set at creation time, and those previously set through 
set_permanent which now simply bump the refcount. And you can no longer 
identify the second set as being permanent.

>>
>> The other way to avoid the race is to implement inc/dec using CAS 
>> instead of raw atomic add/sub.
> 
> That seems expensive.

If you wanted to keep the existing notion of permanent and how it is 
recorded, then it is a way to do that.

David

> Coleen
>>
>> Thanks,
>> David
>>
>>> Thanks
>>> - Ioi
>