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

[David Holmes]

On 26/03/2018 3:18 PM, Ioi Lam wrote:
> 
> 
> On 3/25/18 4:28 PM, David Holmes wrote:
>> 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.
> After my patch, there will be only one type of permanent symbol -- when 
> you create a symbol, you know it will be alive forever (e.g., a symbol 
> requested by the null class loader).
> 
> I am not quite sure about this history of why this is necessary -- 
> except for symbols in the CDS archive. Those are marked as permanent so 
> you won't change its refcount (because it's read only).
> 
> Perhaps it's for performance reasons? There are a lot of Symbols created 
> by the boot loader and are referenced by many classes. So if we don't 
> need to atomically modify their _refcounts then it pays off??
> 
> Or, maybe it's for memory saving -- allocating from arena has less 
> overhead than malloc??
> 
> Anyway, regardless of whether a symbol is permanent or not, the usual 
> rule is -- when you store a symbol (e.g., as an UTF8 into a 
> ConstantPool), you increment its refcount. When you release a symbol 
> (e.g., when said ConstantPool is deallocated), you decrement its refcount.
> 
> The only exception to this rule was Method::log_touched(), which used 
> set_permanent instead. It stores a symbol (in its hashtable) but never 
> releases it (the hashtable persists for the entire VM run time). So in 
> this case, it's perfectly OK to call increment_reference instead.

Okay. That looks likes a special case that perhaps was not completely 
thought through in terms of the race with decrement_refcount. Getting 
rid of it seems fine.

Thanks,
David
-----

> 
>>
>>>>
>>>> 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.
>>
> 
> I think removing set_permanent will make the code cleaner and easier to 
> understand.
> 
> Thanks
> - Ioi
>> David
>>
>>> Coleen
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks
>>>>> - Ioi
>>>
>