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

[Kim Barrett]

> On Aug 25, 2016, at 4:26 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> On 8/24/16 11:52 PM, Kim Barrett wrote:
>>> On Aug 24, 2016, at 8:46 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> 
>>> 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
>> Yes.  I remembered that technique shortly after hitting send.
>> Multiplying a signed value doesn't work (is UB) in the general case
>> because of overflow, but we know the value ranges here are safe from
>> that.
>> 
>>>    jshort(new_value / 0x10000)
>> No, because under 2s complement arithmetic, arithmetic right shift of
>> a negative number is not necessarily equivalent to division by the
>> corresponding power of 2.  [See, for example, "Arithmetic shifting
>> considered harmful", Guy Steele, ACM SIGPLAN Notices, 11/1977.]
>> 
>> Consider the 32bit value with all 1s in the upper 16 bits, and a
>> non-zero value in the lower 16 bits.  If division is truncate, which
>> it is defined to be for C99/C++11 (*), that value / 0x10000 == 0,
>> rather than the desired -1.  Clear the low 16bits first and then
>> divide, and I think it works for the case at hand, though I haven't
>> proved it.  But pragmatically we’re probably better off assuming
>> right shift works as expected, though perhaps in a wrapper to
>> help indicate we’ve actually thought about the issue.
>> 
>> (*) For C89/C++98 the rounding of division involving negative operands
>> is implementation defined, perhaps in part to allow the "optimization"
>> of division by a power of 2 to arithmetic right shift.
>> 
>>> Does C/C++ preserve signs when multiplying/dividing with a positive constant?
> 
> Hi Kim,
> 
> I looked for the use of >> in our source code:
> 
> globalDefinitions.hpp:
> inline jint high(jlong value)                    { return jint(value >> 32); }
> 
> sharedRuntimeTrans.cpp:
> static double __ieee754_log(double x) {
>  double hfsq,f,s,z,R,w,t1,t2,dk;
>  int k,hx,i,j;
>  ...
>  k += (hx>>20)-1023;
> 
> So maybe we already assume that >> "does the right thing" for us?

Yes, that's what I expected to see.

> -------------------------------
> 
> 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.

> 
> inline int extract_low_short_from_int(jint x) {
>  return x & 0xffff;
> }
> inline int extract_high_short_from_int(jint x) {
>  return (x >> 16) & 0xffff;
> }
> inline int build_int_from_shorts( jushort low, jushort high ) {
>  return ((int)((unsigned int)high << 16) | (unsigned int)low);
> }
> 
> I am thinking of adding:
> 
> inline int extract_signed_high_short_from_int(jint x) {
>  if (x >= 0) {
>    return (x >> 16) & 0xffff;
>  } else {
>    return int((unsigned int)x >> 16) | 0xffff0000);
>  }
> }
> 
> inline int build_int_from_shorts(jshort low, jshort high) {
>  return build_int_from_shorts(jushort(low) & 0xffff, jushort(high) & 0xffff);
> }
> 
> What do you think?
> - Ioi