RFR(XS): 8205172: 32 bit build broken

[David Holmes]

Hi Martin,

On 19/06/2018 4:58 PM, Doerr, Martin wrote:
> Hi David,
> 
> it's not a compiler bug. The problem is that "const intptr_t OneBit     =  1;" uses intptr_t which is 32 bit on 32 bit platforms.
> We can't shift it by 48. Seems like the other usages of right_n_bits only use less than 32.

No we can't, but we're also not going to try as we won't take that path 
on a 32-bit system. So we're getting a compiler warning about code that 
the compiler thinks might be executed even though it actually won't.

In this case though the fix is correct as we're always dealing with 
64-bit and those macros depend on the pointer-size.

> I just noticed that there's more to fix:
> --- a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c  Mon Jun 18 16:00:23 2018 +0200
> +++ b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.c  Tue Jun 19 08:53:26 2018 +0200
> @@ -459,7 +459,7 @@
>       live_object = (ObjectTrace*) malloc(sizeof(*live_object));
>       live_object->frames = allocated_frames;
>       live_object->frame_count = count;
> -    live_object->size = size;
> +    live_object->size = (size_t)size;

It's not obvious that is the right fix as the incoming "size" is 64-bit. 
I think strictly speaking this:

typedef struct _ObjectTrace{
   jweak object;
   size_t size;

Should be using a 64-bit type for size.

David
-----

>       live_object->thread = thread;
>       live_object->object = (*jni)->NewWeakGlobalRef(jni, object);
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 19. Juni 2018 07:53
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev developers (hotspot-dev at openjdk.java.net) <hotspot-dev at openjdk.java.net>; Roland Westrelin (roland.westrelin at oracle.com) <roland.westrelin at oracle.com>; JC Beyler <jcbeyler at google.com>
> Subject: Re: RFR(XS): 8205172: 32 bit build broken
> 
> On 19/06/2018 12:10 AM, Doerr, Martin wrote:
>> Hi,
>>
>> 32 bit build is currently broken due to:
>> "trap_mask": jdk/src/hotspot/share/oops/methodData.hpp(142) : warning C4293: '<<' : shift count negative or too big, undefined behavior
>> "PrngModMask": jdk/src/hotspot/share/runtime/threadHeapSampler.cpp(50) : warning C4293: '<<' : shift count negative or too big, undefined behavior
> 
>       const uint64_t PrngModPower = 48;
> !   const uint64_t PrngModMask = right_n_bits(PrngModPower);
> 
> So right_n_bits(48) should expand to:
> 
> (nth_bit(48) - 1)
> 
> where nth_bit is:
> 
> (((n) >= BitsPerWord) ? 0 : (OneBit << (n)))
> 
> so the compiler is complaining that (OneBit << 48) is undefined, but the
> conditional operator will not execute that path (as BitsPerWord == 32).
> That seems like a compiler bug to me. :(
> 
> David
> -----
> 
>> Please review this small fix:
>> http://cr.openjdk.java.net/~mdoerr/8205172_32bit_build/webrev.00/
>>
>> Best regards,
>> Martin
>>