RFR: 8205677: [8u] casts and type change for 8u to enable later Windows compilers

[David Holmes]

Hi Kevin,

All seems fine. One comment below.

On 6/07/2018 10:27 PM, Kevin Walls wrote:
> Hi,
> 
> I'd like to update my review request:
> 
> Item #3, was solved by 8150426 so no change required.
> 
> Item #5, changing the return type of the method is not necessary.  The 
> error was in src/share/vm/runtime/memprofiler.cpp and can be solved with 
> a cast on the offending line.  The result of a long divided by a size_t 
> can be cast to a long, and that minimal change is  accepted in all the 
> builds:
> 
> -          OopMapCache::memory_usage() / K);
> +          (long) (OopMapCache::memory_usage() / K));

I actually thing the correct thing to do is change the return type. We 
should generally never use a raw C long in hotspot due to its varying 
size depending on platform (ILP32, LP64, LLP64). But that goes beyond 
addressing a compiler warning.

Thanks,
David

> Additionally, I've uncovered one more required cast, so item #6 is 
> src/share/vm/classfile/classFileParser.cpp changing an (int) cast to a 
> (u2).
> 
> New webrev at http://cr.openjdk.java.net/~kevinw/8205677/webrev.01/
> 
> Progress report: with these changes, 8206454 which I just put out for 
> review, and one other change re: vnsprintf, the open jdk8u builds with 
> VS2017.
> 
> Many thanks!
> Kevin
> 
> 
> 
> 
> 
> On 05/07/2018 09:46, Kevin Walls wrote:
>> Hi,
>>
>> I'd like to get a review of:
>>
>> 8205677: [8u] casts and type change for 8u to enable later Windows 
>> compilers
>> https://bugs.openjdk.java.net/browse/JDK-8205677
>>
>> This is a collection of new small changes aiming to permit 8u to build 
>> with later Windows compilers (and also with the current VS2010).
>>
>> The changes are:
>>
>> 8u webrev: http://cr.openjdk.java.net/~kevinw/8205677/webrev.00/
>>
>> Notes:
>>
>> 1
>> hotspot\src\share\vm\classfile\altHashing.cpp(227): warning C4838: 
>> conversion from 'unsigned int' to 'const jint' requires a narrowing 
>> conversion
>>
>> (jint) casts are present in the later JDK 
>> test/hotspot/gtest/classfile/test_AltHashing.cpp and look like they 
>> were in JDK9 from the start.
>>
>> 2
>> hotspot\src\share\vm\gc_implementation\parallelScavenge\gcTaskManager.cpp(1038): 
>> warning C4312: 'type cast': conversion from 'unsigned int' to 'Monitor 
>> *' of greater size
>>
>> -  _monitor = (Monitor*) 0xDEAD000F;
>> +  _monitor = (Monitor*) (uintptr_t) 0xDEAD000F;
>>
>> 3
>> ..\hotspot\src\share\vm\oops/typeArrayOop.hpp(132): error C2440: '=': 
>> cannot convert from 'jlong' to 'jlong *'
>> ..\hotspot\src\share\vm\oops/typeArrayOop.hpp(132): note: Conversion 
>> from integral type to pointer type requires reinterpret_cast, C-style 
>> cast or function-style cast
>>
>> -    *long_at_addr(which) = (long)contents;
>> +    *long_at_addr(which) = (jlong)contents;
>>
>>
>> 4
>> ...\hotspot\src\share\vm\gc_implementation\concurrentMarkSweep\concurrentMarkSweepGeneration.cpp(6833): 
>> warning C4334: '<<': result of 32-bit shift implicitly converted to 64 
>> bits (was 64-bit shift intended?)
>>
>> Like in 8197864 where we cast a 1 to an intptr_t before the shift.
>>
>>    assert(mr.end() == (HeapWord*)round_to((intptr_t)mr.end(),
>> -                        (1 << (_shifter+LogHeapWordSize))),
>> +                        ((intptr_t) 1 << (_shifter+LogHeapWordSize))),
>>
>>
>> 5
>> hotspot\src\share\vm\runtime\memprofiler.cpp(129): warning C4477: 
>> 'fprintf' : format string '%6ld' requires an argument of type 'long', 
>> but variadic argument 3 has type '::size_t'
>>
>> OopMapCache::memory_size() is only used in one place, and only when 
>> NOTPRODUCT.  Here in 8u, we can make it return a size_t and compile 
>> without error.
>>
>> In jdk10 onwards, after 8186042, this line in memprofiler.cpp 
>> hard-codes a 0L instead of OopMapCache::memory_size(), as the tracking 
>> of _total_memory_size was removed, but the print was kept to the same 
>> format.  I don't think we would remove that statistic in an update
>>
>>
>> Thanks
>> Kevin
>>
>>
>>
>>
>>
>>
>>
>