RFR (XS) 8161280 - assert failed: reference count underflow for symbol

[Ioi Lam]

Hi Kim,

Thanks for pointing out the problems with the shift operators. I never 
knew that!

Since I am shifting only by 16, can change the expressions to these?

     jint(add_value) * 0x10000
     jshort(new_value / 0x10000)

Does C/C++ preserve signs when multiplying/dividing with a positive 
constant?

Thanks
- Ioi


On 8/24/16 5:06 PM, Kim Barrett wrote:
>> On Aug 24, 2016, at 5:50 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 25/08/2016 7:46 AM, Kim Barrett wrote:
>>>> On Aug 24, 2016, at 3:01 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>
>>>> Hi David,
>>>>
>>>> Here's an updated version that added Atomic::add(jshort*, jshort) as you suggested.
>>>>
>>>> To appease the "unused" warnings, I just added (void)new_value.
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk9/8161280-symbol-refcount-underflow.v03/
>>>>
>>>> I am running RBT with "--test hotspot/test/:hotspot_all,vm.parallel_class_loading,vm.runtime.testlist" to make sure everything works.
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/atomic.hpp
>>> 211   jint new_value = Atomic::add(add_value << 16, (volatile jint*)(dest-1));
>>> 214   jint new_value = Atomic::add(add_value << 16, (volatile jint*)(dest));
>>>
>>> Left-shift of a signed negative value is undefined behavior.
>> Okay so how do we fix that? It seems pretty obvious/simple what we want to do. Do we just cast to unsigned, shift and cast back?
>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/atomic.hpp
>>> 216   return (jshort)(new_value >> 16); // preserves sign
>>>
>>> Right-shift of a signed negative value is implementation-defined. It
>>> may or may not sign-extend. (gcc defines it as sign-extending; I have
>>> no idea about other compilers.)
>> Ditto.
> Unfortunately, I don't have a particularly good answer.  This is a
> rather ugly corner of C/C++.
>
> One option might be to change the increment and decrement operations
> to use unsigned arithmetic with unsigned range checks that correspond
> to the ranges of interest. This would need Atomic::add for jushort
> instead. This should dodge all the signed arithmetic issues. To help
> with this we might extract into a helper the "safe" unsigned to signed
> conversion support from JAVA_INTEGER_OP. This is clumsy and somewhat
> obfuscated.
>
> Alternatively, we can try other mechanisms for working around the
> signed arithmetic issues. For left-shift, the simplest solution might
> be to add JAVA_INTEGER_OP for << to the block of such near the end of
> globalDefinitions.hpp, e.g.
>
> JAVA_INTEGER_OP(<<, java_shift_left, jint, juint)
> JAVA_INTEGER_OP(<<, java_shift_left, jlong, julong)
>
> and update the comments describing these operations, since these don't
> wrap, they just silently discard overflow. And that isn't a constant
> expression (until we can use C++11 constexpr (might require C++14
> constexpr)), which limits where we can use it.
>
> That's not entirely nice, since it expands the usage of these
> operations from "emulate Java operations" to more generally working
> around the specification of C/C++ arithmetic.
>
> Right-shift doesn't have that option. I don't know of a portable and
> reliably efficient way to do a sign-extending right shift. "Hacker's
> Delight" provides several formulas for it, but they all take 5-6
> instructions. I wasn't able to provoke gcc into recognizing any of
> them and generating the desired single instruction. Other compilers
> might do better.
>
> A pragmatic answer might be to just assume all the platforms we care
> about are sign-extending (which is likely what we've been doing all
> along), and add a little startup test to verify that assumption. If
> the test trips, then figure out what to do.
>