RFR (M) 8207359: Make SymbolTable increment_refcount disallow zero

[coleen.phillimore at oracle.com]

On 7/18/18 6:14 PM, Kim Barrett wrote:
>> On Jul 17, 2018, at 11:51 AM, coleen.phillimore at oracle.com wrote:
>>
>> Summary: Use cmpxchg for non permanent symbol refcounting, and pack refcount and length into an int.
>>
>> This is a precurser change to the concurrent SymbolTable change. Zeroed refcounted entries can be deleted at anytime so they cannot be allowed to be zero in runtime code.  Thanks to Kim for writing the packing function and helping me avoid undefined behavior.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8207359.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8207359
>>
>> Tested with solaris ptrace helper, mach5 tier1-5 including solaris. Added multithreaded gtest which exercises the code.
>>
>> Thanks,
>> Coleen
> ------------------------------------------------------------------------------
> src/hotspot/os/solaris/dtrace/jhelper.d
>   467       OFFSET_Symbol_length_and_refcount);
>   474       OFFSET_Symbol_length_and_refcount);
>   483       OFFSET_Symbol_length_and_refcount);
>
> I think these only work on a big-endian platform, and are making
> further assumptions about the Symbol implementation. I have no idea
> what, if anything, to do about that here. At the very least, some
> comments seem warranted.
>
> Similarly in java.base/solaris/native/libjvm_db/libjvm_db.c, though
> some comments were provided there.

Yes, I can add a comment, like:

   /* Because sparc is big endian, the top half length is at the correct 
offset. */

I don't know the changes in dtrace language.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/symbol.cpp
>   233 void Symbol::decrement_refcount() {
>   234   if (refcount() != PERM_REFCOUNT) { // not a permanent symbol
>   235     int new_value = Atomic::sub((uint32_t)1, &_length_and_refcount);
>
> This isn't safe, and can lose counts. Consider refcount is currently
> PERM-1. Decrement checks and finds it's not PERM. Then three other
> threads add references, one setting to PERM and two others leaving it
> at PERM. Now decrement does the sub back to PERM-1, discarding two
> valid references.

I've fixed decrement_refcount() to use a CAS loop also.  Thank you for 
finding and discussing this problem.  I'll post a new webrev after testing.
>
> ------------------------------------------------------------------------------
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Symbol.java
>    78   public long   getLength() {
>    79     int i = (int)length.getValue(this.addr);
>    80     return (i >> 16);
>    81   }
>
> Consider a symbol whose length is in the range [2^15, 2^16). The cast
> result is implementation defined. The right shift of a negative value
> is implemention defined, and might (or might not) return a negative value.

There is no unsigned int in Java, so I added back the & 0xffff which 
seems to work, maybe with less undefined behavior (?)

   public long getLength() {
     long i = length.getValue(this.addr);
     return (i >> 16) & 0xffff;
   }

>
> ------------------------------------------------------------------------------
> test/hotspot/gtest/classfile/test_symbolTable.cpp
>    79   // Test overflowing refcount making symbol permanent
>    80   Symbol* bigsym = SymbolTable::new_symbol("bigsym", CATCH);
>
> Add another check after that a PERM_REFCOUNT value is sticky against
> decrement.

Ok.

>
> ------------------------------------------------------------------------------
> test/hotspot/gtest/threadHelper.inline.hpp
>    24 #ifndef GTEST_THREAD_HELPER_INLINE_HPP
>
> s/THREAD_HELPER/THREADHELPER/ to match the usual Hotspot naming.
Fixed.


> ------------------------------------------------------------------------------
>

Thanks for the thorough review.
Coleen