RFR: 8205677: [8u] casts and type change for 8u to enable later Windows compilers
kevin.walls at oracle.com
Tue Jul 10 09:58:14 UTC 2018
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.
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
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:
>> 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.
>> 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!
>> On 05/07/2018 09:46, Kevin Walls wrote:
>>> I'd like to get a review of:
>>> 8205677: [8u] casts and type change for 8u to enable later Windows
>>> 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/
>>> hotspot\src\share\vm\classfile\altHashing.cpp(227): warning C4838:
>>> conversion from 'unsigned int' to 'const jint' requires a narrowing
>>> (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.
>>> warning C4312: 'type cast': conversion from 'unsigned int' to
>>> 'Monitor *' of greater size
>>> - _monitor = (Monitor*) 0xDEAD000F;
>>> + _monitor = (Monitor*) (uintptr_t) 0xDEAD000F;
>>> ..\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;
>>> 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))),
>>> 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
More information about the hotspot-dev