RFR(XS): 8205172: 32 bit build broken

[Doerr, Martin]

Thanks for reviewing. I've pushed it.

Best regards,
Martin


-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Dienstag, 19. Juni 2018 12:08
To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev developers (hotspot-dev at openjdk.java.net) <hotspot-dev at openjdk.java.net>; JC Beyler <jcbeyler at google.com>
Subject: Re: RFR(XS): 8205172: 32 bit build broken

On 19/06/2018 6:28 PM, Doerr, Martin wrote:
> Hi David,
> 
> thanks for reviewing and for the quick response.
> 
> I think it's good that the compiler warns about undefined subexpressions even when the result is not used.

Not if you have to change the code to work around a warning. :) Static 
analysis tools often are not smart enough to see the complete picture.

> Anyway, the fix is needed to get a correct 64 bit mask as you have already mentioned.
> 
> I'm ok with using jlong as type for the size field. This is safe (even though I think sizes which don't fit into 32 bit should never occur on 32 bit platforms, but I'm not really familiar with this code).

The event is specified to provide a size that is a jlong so one has to 
assume that such sizes are anticipated to occur. For the test we can 
obviously control that so it's not such a concern, but still best that 
the correct sized types flow through the code.

> Here's the new webrev:
> http://cr.openjdk.java.net/~mdoerr/8205172_32bit_build/webrev.01/

Looks good.

Thanks,
David

> Thanks,
> Martin
> 
> 
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 19. Juni 2018 09:18
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev developers (hotspot-dev at openjdk.java.net) <hotspot-dev at openjdk.java.net>; JC Beyler <jcbeyler at google.com>
> Subject: Re: RFR(XS): 8205172: 32 bit build broken
> 
> 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
>>>