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

[Kim Barrett]

> On Aug 28, 2016, at 8:25 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 8/26/16 11:05 AM, Kim Barrett wrote:
>>> On Aug 26, 2016, at 9:34 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>> Since I am doing something very specific (setting/extracting the top 16 bits of a jint), I am a bit hesitant to add routines for general shifting. globalDefinitions.hpp has these:
>>>> The suggestion of using a wrapper was a "perhaps", and not really
>>>> intended for you to deal with while addressing the problem at hand.
>>>> Sorry I was confusing about that.
>>>> 
>>>> If we do something along that line (as a separate project), I suggest
>>>> we keep with the names we've already got for similar operations,
>>>> e.g. use java_shift_{left,right} to be consistent with java_add and
>>>> friends.
>>> Hi Kim,
>>> 
>>> Thanks for the clarification.
>>> 
>>> My RBT tests passed, so I will check in the code as is in my last webrev using the >> and << operators. I'll leave the general problem of java_shift_left/right as a future improvement.
>>> 
>>> Thanks
>>> - Ioi
>>> 
>> It seems my attempt at clarification has led to further confusion.
>> 
>> Recent discussion in this thread has been focused on the right shift,
>> e.g. assuming it "does the right thing".
>> 
>> The left shift is *broken*.  Recent versions of gcc *will* do
>> something other than what is being expected.  We've already seen
>> reports of (and fixed) problems encountered by folks using gcc6 for
>> exactly this sort of thing.  See, for example, JDK-8157758.
>> 
>> In the specific case at hand, the compiler can trivially prove that
>> undefined behavior is being invoked, because of the constant -1 being
>> passed to an inline function where the shift occurs. What it does from
>> there is anyone's guess; gcc6 seems to be treating such things as
>> unreachable code and optimizing accordingly.
>> 
>> So not fixing the left shift is just leaving a land mine for someone
>> else to step on.  Please don't do that.
> 
> Hi Kim,
> 
> I've already pushed my changes, so I need to fix this in a separate bug ID.
> Will something like this work?
> 
> inline jshort Atomic::add(jshort add_value, volatile jshort* dest) {
> - jint new_value = Atomic::add(add_value << 16, (volatile jint*)(dest-1));
> + jint int_add_value = jint(juint(add_value) << 16);

No, that’s just a different path to undefined behavior.  The large *positive* value
resulting from juint(add_value) << 16 exceeds the positive jint range.

The way to twiddle the bit representation is the reinterpret_cast of a reference
trick used in the java_xxx functions in globalDefinitions.hpp.  But simpler in this
case, since we know the value ranges, is to just multiply by 1 << 16 rather than
left-shifting by 16.

And as Andrew pointed out, it looks like this might not be as urgent as I thought,
since it seems gcc (even recent versions) is only treating left shift of negative in
a problematic way in constant expression contexts.

> + jint new_value = Atomic::add(int_add_value, (volatile jint*)(dest-1));