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

[David Holmes]

On 10/07/2018 7:58 PM, Kevin Walls wrote:
> Thanks David --
> 
> #5: My initial webrev changing only the return type of 
> OopMapCache::memory_size() isn't going to work everywhere, the VS2017 
> compiler doesn't accept a size_t into a printf %l format, so we would 
> still want a cast to make that line work.

Well we should never be using %l either.

> But it is odd that the fprintf line uses UINTX_FORMAT_W for the two 
> items which are the result of "size_t / size_t", but then uses %l rather 
> than INTX_FORMAT_W for the result of "long / size_t".
> 
> INTX_FORMAT_W does work, but as it's a size, and shouldn't really be 
> signed anyway, making OopMapCache::memory_usage()  return size_t and 
> changing the fprintf format to UINTX_FORMAT_W to match the others makes 
> things consistent.
> 
> http://cr.openjdk.java.net/~kevinw/8205677/webrev.02/

That looks good to me. Use of size_t makes more sense to me.

Thanks,
David

> Thanks!
> Kevin
> 
> 
> 
> 
> On 09/07/2018 05:08, David Holmes wrote:
>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>